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

[m3ninx] Avoid cloning of roaring bitmap in conjunctionSearcher #3948

Merged

Conversation

linasm
Copy link
Collaborator

@linasm linasm commented Nov 25, 2021

What this PR does / why we need it:
conjunctionSearcher.Search was always making a deep copy (Clone) of the first PostingsList, even though it was not necessary when there was more than one PostingsList in play. It was not necessary because both Intersect and Difference construct a new roaring bitmap under the hood (there are no in-place implementations for Intersect and Difference).

I have added a call to CloneAsMutable at the end of conjunctionSearcher.Search, it will only be executed if there were no Intersect/Difference calls made. I might have been too paranoid and could have avoided this because conjunctionSearcher.Search returns immutable List (so technically there would be no need to clone the input), but this all design does not fully embrace immutability (e.g. same object implements both mutable and immutable interfaces), which makes it very fragile, so I decided to err on the safe side.

In practice, I expect this clone in the end to almost never happen. Because even for a very basic Prometheus query of the form foo{bar="x"} you already have an intersection of 2 search predicates, meaning that the cloning gets avoided.

Note that the savings on heap size would not be very significant (I believe because those objects are mostly short lived):
Screenshot 2021-11-25 at 12 32 53

But reduction of allocation/GC pressure in some use cases might be significant:
Screenshot 2021-11-25 at 12 39 39

Special notes for your reviewer:
To make the immutable vs in-place operations more explicit, I have moved Intersect and Difference from MutableList to the immutable List interface and changed them to return the resulting value instead of doing a shallow mutation.
Also, renamed Union/UnionMany to UnionInPlace/UnionManyInPlace, to better reflect the semantics (and to match the names of the underlying bitmap methods used).
Also, renamed List.Clone to List.CloneAsMutable because it is returning a MutableList, so that it is not a proper clone method.

Does this PR introduce a user-facing and/or backwards incompatible change?:
NONE

Does this PR require updating code package or user-facing documentation?:
NONE

@codecov
Copy link

codecov bot commented Nov 25, 2021

Codecov Report

Merging #3948 (f1ba705) into master (fc654f1) will decrease coverage by 0.2%.
The diff coverage is n/a.

❗ Current head f1ba705 differs from pull request most recent head 5a70a6e. Consider uploading reports for the commit 5a70a6e to get more accurate results

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #3948     +/-   ##
========================================
- Coverage    56.9%   56.7%   -0.3%     
========================================
  Files         551     555      +4     
  Lines       63126   63406    +280     
========================================
+ Hits        35947   35975     +28     
- Misses      24003   24237    +234     
- Partials     3176    3194     +18     
Flag Coverage Δ
aggregator 62.5% <ø> (-0.1%) ⬇️
cluster ∅ <ø> (∅)
collector 58.4% <ø> (?)
dbnode 60.4% <ø> (-0.3%) ⬇️
m3em 46.4% <ø> (ø)
metrics 19.7% <ø> (ø)
msg 74.1% <ø> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2eace16...5a70a6e. Read the comment docs.

@linasm linasm requested a review from vdarulis November 25, 2021 16:24
Copy link
Collaborator

@Antanukas Antanukas left a comment

Choose a reason for hiding this comment

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

LGTM. Indeed cloning looks necessary if we do at least one intersection/union/difference operation.

Copy link
Collaborator

@soundvibe soundvibe left a comment

Choose a reason for hiding this comment

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

LGTM from the code perspective, but don't have deep understanding of this codebase.

@linasm linasm enabled auto-merge (squash) December 8, 2021 08:03
@linasm linasm merged commit 982c6d7 into master Dec 8, 2021
@linasm linasm deleted the linasm/avoid-roaringbitmap-clone-in-conjunction-searcher branch December 8, 2021 08:19
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.

4 participants