Repository navigation
Avoid redundant worker copies in process class special keys - #14260
tclinkenbeard-oai merged 3 commits into
Conversation
tclinkenbeard-oai
left a comment
There was a problem hiding this comment.
Generated by Codex.
What problem is this PR trying to solve? (ELI10)
When someone asks FoundationDB for its workers’ process classes—or where those classes came from—the code makes an unnecessary extra copy of the worker list.
Illustratively, with 1,000 workers, each reader copies another 1,000 records before sorting them. The answers are already correct; the extra copy costs memory and CPU.
How does this PR solve the problem? (ELI10 and review walkthrough)
Each reader keeps one local worker list and uses it directly, removing the extra copy.
- Fetch and organize workers. Both getProcessClassActor and getProcessClassSourceActor initialize an owning, mutable vector from the awaited result. The address sorting and duplicate removal are unchanged.
- Return class names. The class reader’s result construction still filters the requested range, stores strings in the result’s memory arena, and applies the transaction’s pending writes.
- Return class sources. The source reader’s result construction still retains the memory backing its returned keys and values. Removing the extra worker list does not shorten their lifetime.
What is it trying to do?
Remove redundant vector copies and obsolete “strip const” comments from two special-key readers, preserving their behavior.
Is it correct?
Yes, by inspection of head 5472664021ce against base 6a56b0a1cd6a.
The important distinction is that awaiting the future returns a const reference, but the explicit vector declaration still copies that result into owned storage. Only the subsequent redundant copy disappears.
This removes O(n) copying per reader invocation. Sorting, waits, cancellation, error propagation, assertions, output formats, and serialization remain unchanged.
No builds or tests were run during this review. Current CI has passing clang-format and clang-tidy checks; eight checks remain pending, with no failures.
Are there bugs?
I did not find any correctness bugs.
Are there omissions?
None that I think block this. Existing behavioral coverage checks class values and ordering and source values and ordering. This change does not introduce a distinct behavioral gap requiring another test.
Are there better ways of doing things?
The narrow change is appropriate. Further refactoring is unnecessary.
Should this CL be LGTMd?
Yes, LGTM. Ownership, asynchronous behavior, output lifetimes, and existing coverage support the change. The remaining uncertainty is execution across the configurations whose CI checks are still pending.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ove-worker-vector-copies
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ove-worker-vector-copies
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS 14.x
|
The process-class and process-class-source special-key readers each copy the worker vector immediately after receiving it. Initialize
workersdirectly fromgetWorkers()and remove the redundant local and stale "strip const" comment in both functions.Sorting, duplicate removal, and result construction are unchanged.
Validation: built
fdbclient_test;fdbclient_test --seed 1passed all 125 tests. Formatting and whitespace checks passed.