-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Optimize resolver collection performance in CommitProxyServer #12391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
a689fe7
to
88d7705
Compare
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
a94697d
to
3ea034e
Compare
Result of foundationdb-pr-clang-ide 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 on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
3ea034e
to
54ef22f
Compare
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
54ef22f
to
f7459e6
Compare
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr-macos-m1 on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Result of foundationdb-pr-macos on macOS Ventura 13.x
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-clang-arm on Linux CentOS 7
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Result of foundationdb-pr-clang on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr on Linux RHEL 9
|
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
- Can you add 100K results to the PR?
- Is there any data (e.g. benchmark) showing the improvement as a result of this change?
} | ||
} | ||
ASSERT(resolvers.size()); | ||
for (int resolver : resolvers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm: functionally, before/after this change, this order of traversal will be different, right? The previous traversal was ordered by resolver ids. Looks like we don't rely on that order for correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, this function and its callers do not rely on traverse by resolver id order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. I can accept the PR once you answer #12391 (review).
Added 100K pass to header. I did not include a benchmark in the PR because the improvement is algorithmically sound and the benefits are predictable (also the change carries minimal risk), but below are results N/K where N=number of resolvers and K=how many times each resolver ID appears on average in the test data.
|
@spraza this got merged accidentally before you had a chance to look at the perf results I posted. |
This is an optimization to
addReadConflictRanges
andaddWriteConflictRanges
. These functions walk every conflict range in the transaction, consult ProxyCommitData::keyResolvers to discover which resolvers own the shards coveringthat span, and then copy the range into each resolver’s ResolveTransactionBatchRequest (aligned with the snapshot seen by the transaction.)
The optimization replaces std::set with a std::vector and boolean array-based for resolver collections.
100K pass
20250930-035253-dlambrig-6b2a146901aecfa5
Code-Reviewer Section
The general pull request guidelines can be found here.
Please check each of the following things and check all boxes before accepting a PR.
For Release-Branches
If this PR is made against a release-branch, please also check the following:
release-branch
ormain
if this is the youngest branch)