Repository navigation
perf: compact runtime and storage member layouts - #317
thweetkomputer wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change reorders members in runtime and storage structures and updates related aggregate initializers without changing their values. It also updates the recorded ChangesMember Layout and Initializer Alignment
bycorf Subproject Reference
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 14 files. (2 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly and concisely describes the main change: compacting runtime and storage member layouts for performance. Description check The description includes all required template sections and provides context, behavior changes, implementation details, design decisions, documentation status, test results, risks, rollback, reviewer …
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 18 functions across 14 files. (2 skipped: 1 unsupported, 1 too large.)
✨ Finishing Touches 💡 1
-
📝 Generate docstrings 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
A rabbit checks the fields in rows,
Then hops where each initializer goes.
The scores stay put, the values too,
A fresh commit joins the review queue.
Soft paws mark the change as done.
Comment @coderabbitai help to get the list of available commands.
1 participant
Context
Several frequently retained runtime and storage types place small fields between aligned members, increasing object, vector, and coroutine-frame footprints. This change groups fields so the resulting branch can be benchmarked against Lavik
8028ec3e3and its pinned Bycorf7904686.Behavior before and after
Member types, defaults, request behavior, ownership, and durable/wire encodings are unchanged. Measured AArch64/GCC 13.3 sizes with tracing disabled:
RemoteWorkTask<absl::Status>::promise_typeSizeIoAwaitableOpenFixedFileAwaitableConnectionTxUndoEntryOrderedGroupEntryScanHashMap::RehashMoveTransactionStagingSlotReplicaTransactionArrivalConnectionContext::WatchedKeyRespCommandParserConnectionContextCapturedBlockingNotificationTransactionis 600 B with only the Lavik changes; the combined 568 B includes smaller embedded Bycorf state. Coroutine promises and awaiters also shrink for representative return types/captures; the companion PR reports those instances.Implementation
Design decisions and alternatives
Use ordinary member ordering, without packed layouts, narrower field types, new synchronization, or altered cache-line isolation. Leave the replication flow/snapshot atomic layouts outside this change. Public C++ layout changes require a consistent rebuild of Bycorf and all consumers.
Documentation and comments
Add local comments explaining padding-sensitive ordering. Architecture documentation is unchanged: no core boundary, flow, lifecycle, ownership invariant, or durable/wire format claim changes. Keep this audit's measurements in PR context.
Test plan
Run in the AArch64 development environment:
cmake --build build_debug --target lavik lavik_unit_tests bycorf_task_transfer_check bycorf_connect_timer_check bycorf_connection_storage_check bycorf_backend_selection_check lavik_multi_exec_e2e_test lavik_grouped_ordered_write_e2e_test lavik_replica_abort_reclaim_e2e_test lavik_cluster_replication_manager_integration_test --parallel 6 build_debug/lavik_unit_tests --gtest_output=xml:/tmp/lavik-layout-audit/unit-tests.xml ctest --test-dir build_debug --output-on-failure -j 2 -R '^(bycorf_|lavik_multi_exec_e2e$|lavik_grouped_ordered_write_e2e$|lavik_replica_abort_reclaim_e2e$|lavik_replica_abort_small_stack$|cluster_integration\.replication_manager_control_api$)'pre-commit run --files <changed C++ files>andgit diff --checkpassed in both repositories.Risk assessment
Field ordering changes cache placement and coroutine frame size, so throughput/tail-latency effects require measurement. The coroutine pool's size classes can absorb a
sizeofreduction without reducing actual allocation. All consumers must rebuild; do not mix objects compiled against different layouts.Rollback plan
Revert the two commits and rebuild, restoring the previous Bycorf pin and member layouts. No data migration is needed.
Reviewer guide
Review declaration reordering and initializer changes in the first commit, then the companion Bycorf PR and pin in the second. The Lavik-only commit is
599a9c8cb; combined head isa40703eaa(Bycorf05d14a8). Rungit submodule update --init bycorfafter each checkout so the benchmark uses the corresponding runtime.Follow-up work
Benchmark the baseline, Lavik-only commit, and combined PR with identical Release configuration and workloads. Compare throughput, tail latency, and memory; no performance gain is claimed from size probes alone.
Summary by CodeRabbit
bycorfsubproject reference.Performance isolation follow-up
This PR is back in draft while investigating the reported single-key GET/SET performance regression against main. No cause has been established and no corrective change is claimed.
8028ec3e37904686599a9c8cb790468677af0e2c405d14a8a40703eaa05d14a8The added Bycorf-only comparison branch changes only the submodule pin relative to the exact main baseline. Its server and storage-read translation units pass GCC syntax checking with
-Werror=reorder; this comparison variant has not undergone a full build, runtime test suite, or performance benchmark. Use separate worktrees/build directories, synchronize submodules for each revision, and keep build and benchmark settings identical. Focus the next repeated comparison on the io_uring 1:1 workload before testing individual reversions.