opt/bench: fix benchmark regression #138740
Merged
Conversation
This file contains 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
PR cockroachdb#138641 caused extra allocations for plan gist factories in optimizer benchmarks. These allocations should not be included in benchmark results, so they have been eliminated. Release note: None
|
This change is |
|
yuzefovich
approved these changes
Jan 9, 2025
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @michae2)
…der` For our purposes in base64-encoding plan gists, using `bytes.Buffer` in `Encoder` causes fewer allocations, presumably because of a more aggressive growth algorithm. Release note: None
|
I added a second commit. Will post benchmark results of that commit shortly. |
yuzefovich
approved these changes
Jan 9, 2025
There was a problem hiding this comment.
👍
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
|
Diff between first and second commit: |
|
TFTR! bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Jan 14, 2025
137805: sql/row: use Put instead of CPut when updating value of secondary index r=yuzefovich,stevendanna a=michae2 **sql/row: use Put instead of CPut when updating value of secondary index** When an UPDATE statement changes the value but not the key of a secondary index (e.g. an update to the stored columns of a secondary index) we need to write a new version of the secondary index KV with the new value. We were using a CPutAllowingIfNotExists to do this, which verified that if the KV existed, the expected value was the value before update. But there's no need for this verification. We have other mechanisms to detect a write-write conflict with any other transaction that could have changed the value concurrently. We can simply use a Put to overwrite the previous value. This also matches what we do for the primary index when the PK doesn't change. Epic: None Release note: None --- **sql: change CPutAllowingIfNotExists with nil expValue to CPut** CPutAllowingIfNotExists with empty expValue is equivalent to CPut with empty expValue, so change a few spots to use regular CPut. This almost gets rid of CPutAllowingIfNotExists entirely, but there is at least one spot in backfill (introduced in #138707) that needs to allow for both a non-empty expValue and non-existence of the KV. Also drop "(expecting does not exist)" from CPut tracing, as CPut with empty expValue is now overwhelmingly the most common use of CPut. And this matches the tracing in #138707. Epic: None Release note: None 138243: changefeedccl: fix PTS test r=stevendanna a=asg0451 Fix failing TestPTSRecordProtectsTargetsAndSystemTables test Fixes: #135639 Fixes: #138066 Fixes: #137885 Fixes: #137505 Fixes: #136396 Fixes: #135805 Fixes: #135639 Release note: None 138696: sql: add CHECK EXTERNAL CONNECTION command r=kev-cao a=kev-cao This patch adds the `CHECK EXTERNAL CONNECTION` command and replaces the old `SHOW BACKUP CONNECTION` syntax. Epic: None Release note: None 138740: opt/bench: fix benchmark regression r=mgartner a=mgartner #### opt/bench: fix benchmark regression PR #138641 caused extra allocations for plan gist factories in optimizer benchmarks. These allocations should not be included in benchmark results, so they have been eliminated. Release note: None #### util/base64: use `bytes.Buffer` instead of `strings.Builder` in `Encoder` For our purposes in base64-encoding plan gists, using `bytes.Buffer` in `Encoder` causes fewer allocations, presumably because of a more aggressive growth algorithm. Epic: None Release note: None 138877: opt: reduce allocations when filtering histogram buckets r=mgartner a=mgartner `cat.HistogramBuckets` are now returned and passed by value in `getFilteredBucket` and `(*Histogram).addBucket`, respectively, eliminating some heap allocations. Also, two allocations when building spans from buckets via the `spanBuilder` have been combined into one. The new `(*spanBuilder).init` method simplifies the API by no longer requiring that prefix datums are passed to every invocation of `makeSpanFromBucket`. This also reduces redundant copying of the prefix. Epic: None Release note: None 139029: sql/logictest: disable column family mutations in some cases r=mgartner a=mgartner Random column family mutations are now disabled for `CREATE TABLE` statements with unique, hash-sharded indexes. This prevents the AST from being reserialized with a `UNIQUE` constraint with invalid options, instead of the original `UNIQUE INDEX`. See #65929 and #107398. Epic: None Release note: None 139039: ccl/serverccl: revise `TestTenantVars` cpu time checks r=xinhaoz a=xinhaoz Previously, this test verified that a portion of the test's user cpu time would be less than or equal to the entire tenant user cpu time up to that point. This check is flaky because there's no guarantee that the inactive tenant's cpu time will surpass the test cpu time. We now simply verify that the test cpu times are greater than or equal to the tenant metrics. The test was likely passing before 331596c because the reported tenant cpu time was accounting for the sql server prestart. A tenant's user cpu metrics are tracked from the time the `_status/load` endpoint is registered, and the commit above moved the router setup to occur just after the prestart. Epic: none Fixes: #119329 Release note: None Co-authored-by: Michael Erickson <[email protected]> Co-authored-by: Miles Frankel <[email protected]> Co-authored-by: Kevin Cao <[email protected]> Co-authored-by: Marcus Gartner <[email protected]> Co-authored-by: Xin Hao Zhang <[email protected]>
|
Build failed (retrying...):
|
|
bors r+ |
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.
opt/bench: fix benchmark regression
PR #138641 caused extra allocations for plan gist factories in optimizer
benchmarks. These allocations should not be included in benchmark
results, so they have been eliminated.
Release note: None
util/base64: use
bytes.Bufferinstead ofstrings.BuilderinEncoderFor our purposes in base64-encoding plan gists, using
bytes.BufferinEncodercauses fewer allocations, presumably because of a moreaggressive growth algorithm.
Epic: None
Release note: None