Feature metadata serialization#305
Open
biqar wants to merge 41 commits into
Open
Conversation
Added code to add kv-tags at scale. Also added code to get and verify tags at scale. The later test code will be used to verify the metadata checkpointing after a server restart.
…nd from file instead of using a buffer Also added version control for the bulki-based checkpointing.
Added comment, fixed formatting suggested by GitHub clang-format style checker, brought back PDC_TIMING log from the stable version.
…into feature_metadata-serialization
…into feature_metadata-serialization
Collaborator
Author
Benchmark Summary (10000 objects, 100 tags)Serial
Parallel (
|
| Metric | Previous | New (BUKLI) |
|---|---|---|
| Close Time (s) | 0.85 | 0.45 |
| Restart Time (s) | 0.77 | 0.05 |
Collaborator
Author
Member
|
@biqar a couple of things:
|
Collaborator
Author
|
Todos:
|
Collaborator
Author
Strong Scaling Performance ComparisonFixed work to 100K objects and 100 tags creation and changing the number of parallel servers. Reporting min/max/avg of Checkpoint Time (seconds)
Server Close Time (seconds)
Server Restart Time (seconds)
Observations:
Weak Scaling Performance ComparisonFixed the amount of workload per-worker. Reporting min/max/avg of Checkpoint Time (seconds)
Close Time (seconds)
Restart Time (seconds)
Observations
Issue: Unexpected Checkpoint Overhead After Restart with BULKI SerializerDescriptionThe BULKI Serializer integration introduces unexpected performance overhead in the second checkpoint cycle. Since no new data is added after the restart, both checkpoint times should be roughly equivalent. However, we observe a consistent 29–52% increase in the second checkpoint time compared to the first. Workflow
Expected BehaviorSince no new data is added after the restart, the 1st and 2nd checkpoint times should be approximately equal (as showing in the existing binary serializer based checkpoint time comparison bellow). Observed BehaviorThe 2nd checkpoint time is consistently 29–52% more expensive than the 1st under the BULKI Serializer. BULKI Serializer: Checkpoint Time Comparison (seconds)
Binary Serializer: Checkpoint Time Comparison (seconds)
|
Collaborator
|
@biqar could you time each BULKI operation so we can see what's taking so long on the strong scaling? For instance if the |
Collaborator
Author
Strong Scaling Performance ComparisonFixed work to 100K objects and 100 tags creation and changing the number of parallel servers. Reporting min/max/avg of Server Close Time (seconds)
Server Restart Time (seconds)
Weak Scaling Performance ComparisonFixed the amount of workload per-worker. Reporting min/max/avg of Close Time (seconds)
Restart Time (seconds)
Issue: Unexpected Checkpoint Overhead After Restart with BULKI SerializerDescriptionThe BULKI Serializer integration previously observed an unexpected performance overhead in the second checkpoint cycle. Since no new data is added after the restart, both checkpoint times should be roughly equivalent. However, we previously observe a consistent 29–52% increase in the second checkpoint time compared to the first. The new fix in avoiding recursive size computation have fixed the issue. Workflow
Expected BehaviorSince no new data is added after the restart, the 1st and 2nd checkpoint times should be approximately equal. Observed BehaviorThe BULKI Serializer show similar trend as the old binary serializer. BULKI Serializer: Server Close Time Comparison (seconds)
Binary Serializer: Checkpoint Time Comparison (seconds)
|
Initialize MPI and start the close timer before PDCinit so client setup uses the correct communicator. Drop the extra MPI_Barrier before printing total close time since PDC_Client_close_all_server already synchronizes ranks after the close RPC completes.
Add _incremental and _with_capacity BULKI helpers that update cached sizes in O(1) and pre-allocate array slots, while keeping the original put/append/delete APIs unchanged for existing callers. Switch PDC_Server_checkpoint and transfer-query checkpoint building to the fast path, document the new APIs, and add unit tests for incremental put, append, and delete sizing. Mark remaining serialize/write redundancies in bulki_serde.c for follow-up work.
Introduce serial and MPI scripts that start pdc_server, run a test program with caller-supplied arguments, close the server (triggering checkpoint), then repeat with restart for subsequent programs. Wire kvtag_add_scale and kvtag_get_verify_scale into the default test build and register serial and MPI CTest entries that exercise add/verify across server close and restart (1000 objects, 100 tags).
…into feature_metadata-serialization
Build kvtag_add_scale and kvtag_get_verify_scale as serial target (KVTAG_SCALE_SERIAL) and kvtag_add_scale_mpi / kvtag_get_verify_scale_mpi as MPI targets from the same .c files. Use a per-target macro so global ENABLE_MPI does not force MPI into the serial binaries. Initialize rank and timing for serial runs, and point MPI CTest and benchmark scripts at the _mpi executables.
Sync develop to stable
This reverts commit a772b68.
…into feature_metadata-serialization
… source." This reverts commit d8171d9.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Create checkpoint files with mode 0600 and validate untrusted BULKI restart fields (metadata counts, kvtag key/value sizes, region counts, and histogram nbin) before allocation, matching develop-branch's binary checkpoint safeguards.
Applied changes
1. Checkpoint (PDC_Server_checkpoint)
a. Added shared bounds constants (metadata count, kvtag key/size, region count, histogram nbin).
b. Checkpoint files created with open(..., O_CREAT | O_TRUNC, 0600) + fdopen, matching develop’s permission model.
2. Restart (PDC_Server_restart)
a. Metadata count: reject if count <= 0 or > 10,000,000.
b. KV tag key: null checks; key length capped at 65536 (uses key_ent->count when set).
c. KV tag value: size must be > 0 and ≤ 16 MiB; value_ent->count must match.
d. Regions (metadata + dataserver): reject if n_region < 0 or > 1,000,000.
e. Histogram: nbin must be > 0 and ≤ 65536 before allocation; uses validated nbin for malloc sizes.
Collaborator
Author
|
@TheAssembler1, could you please review commit Due to merge conflicts, I had to remove your security-related changes from The only change I did not carry over is "Cap transfer-query payload size (develop: ≤ 4 GiB)." In the binary-based implementation, this check made sense because it was performed before allocating memory for the serialized payload. With the new implementation, transfer_request_metadata_query is serialized/deserialized using BULKI, so I'm not sure whether the same validation is still appropriate or whether we need a BULKI-aware size validation instead. If you think this validation is still necessary, could you please open an issue and assign it to me? I think it would be better to address it in a follow-up PR so that we can keep this PR focused on migrating from binary-based checkpointing to the BULKI-based implementation. Thanks! Applied changesCheckpoint (PDC_Server_checkpoint)
Restart (PDC_Server_restart)
|
| BULKI_get(dataserver_obj, BULKI_singleton_ENTITY("regions", PDC_STRING)); | ||
|
|
||
| if (ds_regions_array != NULL && ds_regions_array->pdc_type == PDC_BULKI) { | ||
| int ds_n_region = ds_regions_array->count; |
| region_list->data_size = 1; | ||
| for (idx = 0; idx < region_list->ndim; idx++) | ||
| region_list->data_size *= region_list->count[idx]; | ||
| region_list->is_data_ready = 0; |
There was a problem hiding this comment.
Pull request overview
This PR integrates BULKI-based serialization into the PDC server checkpoint/restart path to persist and restore in-memory metadata (including transfer-request query state), and adds new scale-style kvtag tests plus checkpoint/restart test runners to validate the behavior.
Changes:
- Switch server checkpoint/restart from raw
fwrite/freadblobs to a BULKI-serialized checkpoint tree (containers, metadata, regions, transfer-query state). - Extend BULKI with incremental size/accounting + preallocated array helpers and add corresponding serde tests.
- Add kvtag add/verify scale tests and new checkpoint/restart runners (serial + MPI) wired into CTest.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/pdc_server.c | Reworks checkpoint/restart implementation to build/parse a BULKI checkpoint tree and initializes transfer-query restore via BULKI. |
| src/server/pdc_server_region/pdc_server_region_transfer_metadata_query.c | Adds BULKI-based init/checkpoint support for transfer metadata query state. |
| src/server/pdc_server_region/include/pdc_server_region_transfer_metadata_query.h | Exposes new BULKI-based init/checkpoint APIs. |
| src/commons/serde/bulki/include/bulki.h | Adds capacity + incremental append/put/delete APIs for efficient bulk construction. |
| src/commons/serde/bulki/bulki.c | Implements incremental APIs, array capacity handling, and cached-size updates. |
| src/commons/serde/bulki/bulki_serde.c | Adds notes around redundant tree walks and buffer-then-fwrite serialization behavior. |
| src/commons/serde/bulki/bulki_serde_test.c | Adds tests validating incremental size accounting and round-trip correctness. |
| src/tests/tags/kvtag_add_scale.c | New scale test for creating many objects and adding tags (used in checkpoint/restart tests). |
| src/tests/tags/kvtag_get_verify_scale.c | New scale test for reading/verifying tags across many objects (used in checkpoint/restart tests). |
| src/tests/run_checkpoint_restart_test_with_prog_param.sh | New serial checkpoint/restart runner that accepts program+args strings. |
| src/tests/run_checkpoint_restart_mpi_test_with_prog_param.sh | New MPI checkpoint/restart runner that accepts program+args strings. |
| src/tests/CMakeLists.txt | Registers new executables/scripts and adds serial + MPI checkpoint/restart tests. |
| src/api/close_server.c | Minor formatting/whitespace change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+81
to
+104
| #ifdef ENABLE_MPI | ||
| MPI_Init(&argc, &argv); | ||
| MPI_Comm_size(MPI_COMM_WORLD, &proc_num); | ||
| MPI_Comm_rank(MPI_COMM_WORLD, &my_rank); | ||
| #endif | ||
| if (argc < 3) { | ||
| if (my_rank == 0) | ||
| print_usage(argv[0]); | ||
| PGOTO_DONE(FAIL); | ||
| } | ||
| n_obj = atoi(argv[1]); | ||
| n_add_tag = atoi(argv[2]); | ||
|
|
||
| if (n_add_tag > n_obj) { | ||
| if (my_rank == 0) | ||
| LOG_ERROR("n_add_tag larger than n_obj! Exiting...\n"); | ||
| PGOTO_DONE(FAIL); | ||
| } | ||
|
|
||
| assign_work_to_rank(my_rank, proc_num, n_add_tag, &my_add_tag, &my_add_tag_s); | ||
| assign_work_to_rank(my_rank, proc_num, n_obj, &my_obj, &my_obj_s); | ||
|
|
||
| obj_1percent = my_obj / 100; | ||
| tag_1percent = my_add_tag / 100; |
Comment on lines
+141
to
+142
| if (i > 0 && i % obj_1percent == 0) { | ||
| #ifdef ENABLE_MPI |
Comment on lines
+180
to
+187
| if (i % tag_1percent == 0) { | ||
| #ifdef ENABLE_MPI | ||
| MPI_Barrier(MPI_COMM_WORLD); | ||
| percent_time = MPI_Wtime() - stime; | ||
| if (my_rank == 0) { | ||
| int current_percentage = i / tag_1percent; | ||
| int estimated_current_tag_number = n_obj / 100 * current_percentage; | ||
| double tps = estimated_current_tag_number / percent_time; |
Comment on lines
+89
to
+111
| #ifdef ENABLE_MPI | ||
| MPI_Init(&argc, &argv); | ||
| MPI_Comm_size(MPI_COMM_WORLD, &proc_num); | ||
| MPI_Comm_rank(MPI_COMM_WORLD, &my_rank); | ||
| #endif | ||
| if (argc < 3) { | ||
| if (my_rank == 0) | ||
| print_usage(argv[0]); | ||
| PGOTO_DONE(FAIL); | ||
| } | ||
| n_obj = atoi(argv[1]); | ||
| n_query = atoi(argv[2]); | ||
|
|
||
| if (n_query > n_obj) { | ||
| if (my_rank == 0) | ||
| LOG_ERROR("n_query larger than n_obj! Exiting...\n"); | ||
| PGOTO_DONE(FAIL); | ||
| } | ||
|
|
||
| assign_work_to_rank(my_rank, proc_num, n_query, &n_tag, &n_tag_s); | ||
| assign_work_to_rank(my_rank, proc_num, n_obj, &my_obj, &my_obj_s); | ||
|
|
||
| obj_1percent = my_obj / 100; |
Comment on lines
+97
to
+104
| BULKI_Entity *ndim_ent = BULKI_get(obj_bulki, BULKI_singleton_ENTITY("ndim", PDC_STRING)); | ||
| if (ndim_ent == NULL) { | ||
| LOG_ERROR("Missing ndim in checkpoint object\n"); | ||
| PDC_free(obj_pkg); | ||
| continue; | ||
| } | ||
| memcpy(&obj_pkg->ndim, ndim_ent->data, sizeof(int)); | ||
|
|
Comment on lines
+135
to
+146
| BULKI_Entity *offset_size_ent = | ||
| BULKI_get(region_bulki, BULKI_singleton_ENTITY("reg_offset_size", PDC_STRING)); | ||
| if (offset_size_ent != NULL) { | ||
| memcpy(region_pkg->reg_offset, offset_size_ent->data, | ||
| sizeof(uint64_t) * obj_pkg->ndim * 2); | ||
| } | ||
| else { | ||
| LOG_ERROR("Missing reg_offset_size in checkpoint region\n"); | ||
| PDC_free(region_pkg->reg_offset); | ||
| PDC_free(region_pkg); | ||
| continue; | ||
| } |
Add null checks after every required BULKI_get() call in PDC_Server_restart() so malformed or incomplete checkpoint files fail with a clear error instead of dereferencing a null entity pointer.
…into feature_metadata-serialization # Conflicts: # src/server/pdc_server.c
When removing the last remaining key, the function decremented numKeys and then always copied from index numKeys - 1. With numKeys equal to 0, that index underflows and can corrupt memory or crash. Skip the swap when the deleted key is already at the last index. Also reject NULL bulki or key arguments at entry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issues / Pull Requests
Related Issue: 282
Description
Integrate BULKI serializer for pdc server checkpointing.
What changes are proposed in this pull request?
Checklist: