Skip to content

Avoid redundant worker copies in process class special keys - #14260

Merged
tclinkenbeard-oai merged 3 commits into
apple:mainfrom
tclinkenbeard-oai:dev/tclinkenbeard/remove-worker-vector-copies
Oct 11, 2026
Merged

tclinkenbeard-oai merged 3 commits into
apple:mainfrom
tclinkenbeard-oai:dev/tclinkenbeard/remove-worker-vector-copies

Conversation

@tclinkenbeard-oai

Copy link
Copy Markdown
Collaborator

The process-class and process-class-source special-key readers each copy the worker vector immediately after receiving it. Initialize workers directly from getWorkers() 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 1 passed all 125 tests. Formatting and whitespace checks passed.

@tclinkenbeard-oai
tclinkenbeard-oai marked this pull request as ready for review October 9, 2026 17:39

@tclinkenbeard-oai tclinkenbeard-oai left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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.
  2. 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.
  3. 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.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

This comment has been minimized.

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-ide on Linux RHEL 9

  • Commit ID: dc622a7
  • Duration 0:29:54
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos-m1 on macOS 14.x

  • Commit ID: dc622a7
  • Duration 0:34:39
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-arm on Linux RHEL 9

  • Commit ID: dc622a7
  • Duration 0:46:55
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux RHEL 9

  • Commit ID: dc622a7
  • Duration 0:55:21
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang on Linux RHEL 9

  • Commit ID: dc622a7
  • Duration 0:55:32
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

  • Commit ID: dc622a7
  • Duration 1:44:53
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@tclinkenbeard-oai
tclinkenbeard-oai merged commit 4d00964 into apple:main Oct 11, 2026
9 of 10 checks passed
@foundationdb-ci

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos on macOS 14.x

  • Commit ID: dc622a7
  • Duration 5:30:50
  • Result: ❌ FAILED
  • Error: Build has timed out.
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants