Skip to content
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

storage: don't reread value during inline conditional writes #109793

Merged
merged 4 commits into from
Aug 31, 2023

Conversation

nvanbenschoten
Copy link
Member

This commit removes the call to maybeGetValue when performing an inline conditional write. In such cases, we will have already read the value in the call to mvccGetMetadata, so we don't need to do so again.

I'm not aware of any workloads that are sensitive to the performance of conditional inline writes and I also suspect that the positioning of the Pebble iterator was avoiding some work during the second seek, so this is mainly just intended to be a code simplification.

Epic: None
Release note: None

This commit adds a basic test of inline operations to the
TestMVCCHistories test suite.

Epic: None
Release note: None
@nvanbenschoten nvanbenschoten requested a review from a team as a code owner August 31, 2023 04:50
@cockroach-teamcity
Copy link
Member

This change is Reviewable

This commit removes the call to maybeGetValue when performing an inline
conditional write. In such cases, we will have already read the value in
the call to mvccGetMetadata, so we don't need to do so again.

I'm not aware of any workloads that are sensitive to the performance of
conditional inline writes and I also suspect that the positioning of the
Pebble iterator was avoiding some work during the second seek, so this is
mainly just intended to be a code t simplification.

Epic: None
Release note: None
The function is no longer needed. We can inline its one caller and gain
some readability.

Epic: None
Release note: None
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/cleanupInline branch from aa23c7a to d627e48 Compare August 31, 2023 14:28
@nvanbenschoten nvanbenschoten changed the title storage: don't reread value during inline condition writes storage: don't reread value during inline conditional writes Aug 31, 2023
Copy link
Contributor

@itsbilal itsbilal left a comment

Choose a reason for hiding this comment

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

LGTM

This commit uses putBuffer.newMeta when writing an inline value, instead
of putBuffer.meta. This avoids a near-bug, because we were consulting
buf.meta.Deleted a few lines down.

Epic: None
Release note: None
@nvanbenschoten
Copy link
Member Author

TFTR!

bors r=itsbilal

@craig
Copy link
Contributor

craig bot commented Aug 31, 2023

Build succeeded:

@craig craig bot merged commit d435960 into cockroachdb:master Aug 31, 2023
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/cleanupInline branch August 31, 2023 20:27
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