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.

Sign up for GitHub

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

Jump to bottom

storage: remove end-key comparison in ComputeStatsForRange #41899

Closed
petermattis opened this issue Oct 24, 2019 · 2 comments · Fixed by #41896 or #85580
Closed

storage: remove end-key comparison in ComputeStatsForRange #41899

petermattis opened this issue Oct 24, 2019 · 2 comments · Fixed by #41896 or #85580
Labels
C-performance Perf of queries or internals. Solution not expected to change functional behavior. T-storage Storage Team

Comments

@petermattis
Copy link
Collaborator

petermattis commented Oct 24, 2019 •
edited by cockroach-jira-scripts
Loading

On every iteration inside of the main loop in ComputeStatsForRange, a comparison is done against the end key. This is usually unnecessary as the iterator that has been passed to ComputeStatsForRange usually has had an UpperBound set. The performance impact of removing this comparison is large:

name                                      old time/op    new time/op    delta
MVCCComputeStats_Pebble/valueSize=8-16       159ms ± 4%     115ms ± 8%  -27.72%  (p=0.000 n=10+10)
MVCCComputeStats_Pebble/valueSize=32-16      110ms ± 7%      78ms ± 3%  -29.73%  (p=0.000 n=10+10)
MVCCComputeStats_Pebble/valueSize=256-16    35.2ms ±11%    26.5ms ±15%  -24.79%  (p=0.000 n=10+10)

name                                      old speed      new speed      delta
MVCCComputeStats_Pebble/valueSize=8-16     424MB/s ± 4%   586MB/s ± 8%  +38.47%  (p=0.000 n=10+10)
MVCCComputeStats_Pebble/valueSize=32-16    609MB/s ± 8%   866MB/s ± 3%  +42.18%  (p=0.000 n=10+10)
MVCCComputeStats_Pebble/valueSize=256-16  1.91GB/s ±11%  2.55GB/s ±13%  +33.30%  (p=0.000 n=10+10)

The challenge to removing it is that some uses of ComputeStatsForRange use iterators which don't have an upper-bound. For example, engine/bulk.AddSSTable.

Jira issue: CRDB-5409
@petermattis petermattis added the C-performance Perf of queries or internals. Solution not expected to change functional behavior. label Oct 24, 2019
@petermattis petermattis mentioned this issue Oct 24, 2019
Merged
petermattis added a commit to petermattis/cockroach that referenced this issue Oct 24, 2019
@petermattis
storage/engine: fix pebbleBatch.iter reuse
550d483 
`pebbleBatch.NewIterator` was setting `pebbleBatch.iter.inuse = true`,
and then calling `pebbleIterator.init` which was clearing that
field. This was broken by cockroachdb#41859 which refactored how `pebbleBatch`
iterator reuse works.

Added separate cached prefix and normal iterators to
`pebble{Batch,ReadOnly}`. Various bits of higher-level code expect to be
able to have a prefix and normal iterator open at the same time. In
particularly, this comes up during intent resolution. This also mimics
our usage of RocksDB which seems desirable in the short term even though
the semantics of having two cached iterators is slightly odd.

Fixes cockroachdb#41899
@petermattis
Copy link
Collaborator Author

Oops, I specified the wrong issue to close in a PR.

@petermattis petermattis reopened this Oct 24, 2019
@github-actions
Copy link

github-actions bot commented Jun 4, 2021

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
5 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

@jlinder jlinder added the T-storage Storage Team label Jun 16, 2021
@jbowens jbowens changed the title storage/engine: remove end-key comparison in ComputeStatsGo storage: remove end-key comparison in ComputeStatsForRange Jan 4, 2022
@erikgrinaker erikgrinaker mentioned this issue Jul 17, 2022
Closed
@erikgrinaker erikgrinaker mentioned this issue Aug 3, 2022
Merged
@craig craig bot closed this as completed in 9fb8d36 Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-performance Perf of queries or internals. Solution not expected to change functional behavior. T-storage Storage Team
Projects
None yet
Milestone
No milestone
2 participants
@jlinder @petermattis