store/proxy: Deduplicate chunks on StoreAPI level. Recommend chunk sorting for StoreAPI. #2603
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
bwplotka
changed the title
bwplotka
force-pushed
the
issue2303-dedup-same-chunks
branch
from
973ad6a
to
8717698
Compare
bwplotka
changed the title
bwplotka
force-pushed
the
issue2303-dedup-same-chunks
branch
2 times, most recently
from
715a3ed
to
4f3f53d
Compare
|
As expected there is some overhead of this. 3x CPU 1.5x Mem allocs. Overhead was expected but maybe not that significant. NOTE: Dataset does not have ANY duplicates, so we do in this function MORE, to do later, less (: |
|
Most of the mem comes from string comparison and marshalling... I guess we could do better 🤔
|
|
I think the strategy seems sane, but could we add a couple of benchmarks that prove the theory that this does indeed make the typical setup faster? |
|
Ok, I know what's wrong (:
Looks like text comparison sucks. |
…ng for StoreAPI. Also: Merge same series together on proxy level instead select. This allows better dedup efficiency. Partially fixes: #2303 Cases like overlapped data from store and sidecar and 1:1 duplicates are optimized as soon as it's possible. This case was highly visible on GitLab repro data and exists in most of Thanos setup. Signed-off-by: Bartlomiej Plotka <[email protected]>
bwplotka
force-pushed
the
issue2303-dedup-same-chunks
branch
from
4f3f53d
to
da3c83e
Compare
Signed-off-by: Bartlomiej Plotka <[email protected]>
bwplotka
force-pushed
the
issue2303-dedup-same-chunks
branch
from
da3c83e
to
f6004ab
Compare
|
Ok I optimized a lot and I think it's good to go. I can think of more optimizations, but it looks good. NOTE: I just noticed this is the full overlapping benchmark. So it means 100% chunks in those "blocks" are overlapping. That's why comparison was so crucial (: This is normally a rare operation though. I extended tests with non overlap as well. Also this version is with |
bwplotka
force-pushed
the
issue2303-dedup-same-chunks
branch
from
34d9706
to
0ebf5e1
Compare
|
Ok last version uses LESS resources and is FASTER and adds MORE features =D This is compared to the code WITHOUT this PR. Considering this done! |
Signed-off-by: Bartlomiej Plotka <[email protected]>
bwplotka
force-pushed
the
issue2303-dedup-same-chunks
branch
from
0ebf5e1
to
b6579bb
Compare
brancz
approved these changes
May 18, 2020
|
We got hit by my chaining practice again - it was not merged to master after all ): We need some bot helping or I can try modifying titles or something |
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.
Blocked by: #2548
Partially fixes: #2303
It should make certain calls where we have overlapped data from StoreAPIs much faster and more efficient.
Changes:
select. This allows for better dedup efficiency.Signed-off-by: Bartlomiej Plotka [email protected]