*: pass keyspan.Spans by pointer not value#1748
Merged
jbowens merged 1 commit intocockroachdb:masterfrom Jun 9, 2022
Merged
Conversation
Member
|
This change is |
Contributor
Author
|
Looks like I still have an interleaving iter invariant violation to work through. |
nicktrav
approved these changes
Jun 9, 2022
Contributor
nicktrav
left a comment
There was a problem hiding this comment.
Review was fairly mechanical, but 
Still have the other reviews sitting in my inbox 😅
Reviewed 1 of 1 files at r1, 4 of 11 files at r5, 50 of 50 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jbowens)
internal/keyspan/defragment.go line 438 at r6 (raw file):
} if i.iterSpan == nil { // tktk: set i.currValid or something?
Leftovers?
16f18f3 to
b576609
Compare
nicktrav
reviewed
Jun 9, 2022
Contributor
nicktrav
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r8, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @jbowens)
Previously the keyspan.Span type used to represent range deletions and range keys was passed primarily by value, including through the FragmentIterator interface. This was convenient when transforming spans (eg, truncating bounds), but passing them by pointer is enough of a performance win to be worthwhile. Informs cockroachdb/cockroach#82559. ``` name old time/op new time/op delta IteratorSeekNoRangeKeys/batch=false-10 4.28µs ± 1% 2.90µs ± 4% -32.27% (p=0.000 n=19+20) IteratorSeekNoRangeKeys/batch=true-10 8.78µs ± 1% 5.37µs ± 1% -38.86% (p=0.000 n=20+20) name old alloc/op new alloc/op delta IteratorSeekNoRangeKeys/batch=false-10 16.0B ± 0% 16.0B ± 0% ~ (all equal) IteratorSeekNoRangeKeys/batch=true-10 192B ± 0% 128B ± 0% -33.33% (p=0.000 n=20+20) ```
jbowens
commented
Jun 9, 2022
Contributor
Author
jbowens
left a comment
There was a problem hiding this comment.
TFTR!
Reviewable status: 45 of 55 files reviewed, all discussions resolved (waiting on @nicktrav)
jbowens
added a commit
to jbowens/pebble
that referenced
this pull request
Jul 29, 2022
… seeks The defragmenting iterator avoids defragmenting empty spans, because the empty span fragments are not observable by the user. However on seeks, it would still pay the cost of buffering the empty span and stepping the iterator both directions. This was a regression added in cockroachdb#1748, and hidden among that change's own performance wins. Previously, we used `Span.Empty()` in the defragmenting iterator to detect both of the two cases: a span doesn't exist, or a span exists but is empty. Now that nonexistent spans are surfaced as `nil`s, these cases must handled separately. ``` name old time/op new time/op delta IteratorSeekNoRangeKeys/batch=false-10 2.58µs ± 1% 2.49µs ± 1% -3.49% (p=0.000 n=10+10) IteratorSeekNoRangeKeys/batch=true-10 4.87µs ± 1% 4.02µs ± 1% -17.44% (p=0.000 n=10+10) name old alloc/op new alloc/op delta IteratorSeekNoRangeKeys/batch=false-10 80.0B ± 0% 80.0B ± 0% ~ (all equal) IteratorSeekNoRangeKeys/batch=true-10 160B ± 0% 160B ± 0% ~ (all equal) name old allocs/op new allocs/op delta IteratorSeekNoRangeKeys/batch=false-10 2.00 ± 0% 2.00 ± 0% ~ (all equal) IteratorSeekNoRangeKeys/batch=true-10 3.00 ± 0% 3.00 ± 0% ~ (all equal) ```
jbowens
added a commit
that referenced
this pull request
Aug 2, 2022
… seeks The defragmenting iterator avoids defragmenting empty spans, because the empty span fragments are not observable by the user. However on seeks, it would still pay the cost of buffering the empty span and stepping the iterator both directions. This was a regression added in #1748, and hidden among that change's own performance wins. Previously, we used `Span.Empty()` in the defragmenting iterator to detect both of the two cases: a span doesn't exist, or a span exists but is empty. Now that nonexistent spans are surfaced as `nil`s, these cases must handled separately. ``` name old time/op new time/op delta IteratorSeekNoRangeKeys/batch=false-10 2.58µs ± 1% 2.49µs ± 1% -3.49% (p=0.000 n=10+10) IteratorSeekNoRangeKeys/batch=true-10 4.87µs ± 1% 4.02µs ± 1% -17.44% (p=0.000 n=10+10) name old alloc/op new alloc/op delta IteratorSeekNoRangeKeys/batch=false-10 80.0B ± 0% 80.0B ± 0% ~ (all equal) IteratorSeekNoRangeKeys/batch=true-10 160B ± 0% 160B ± 0% ~ (all equal) name old allocs/op new allocs/op delta IteratorSeekNoRangeKeys/batch=false-10 2.00 ± 0% 2.00 ± 0% ~ (all equal) IteratorSeekNoRangeKeys/batch=true-10 3.00 ± 0% 3.00 ± 0% ~ (all equal) ```
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
The first four commits are from #1743 and the fifth is from #1747, and they
should be reviewed in their respective PRs.
Previously the keyspan.Span type used to represent range deletions and range
keys was passed primarily by value, including through the FragmentIterator
interface. This was convenient when transforming spans (eg, truncating bounds),
but passing them by pointer is enough of a performance win to be worthwhile.
Informs cockroachdb/cockroach#82559.