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

LUCENE-9536: Optimize OrdinalMap when one segment contains all distinct values. #1948

Merged
merged 4 commits into from
Nov 2, 2020
Merged

LUCENE-9536: Optimize OrdinalMap when one segment contains all distinct values. #1948

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4f22cba
LUCENE-9536: Optimize OrdinalMap when one segment contains all distin…
jtibshirani Sep 13, 2020
55ac801
Omit static singleton from RAM estimate.
jtibshirani Oct 15, 2020
e9d7b94
Clarify comment.
jtibshirani Oct 15, 2020
a0330e3
Add test that covers the optomization.
jtibshirani Oct 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 26 additions & 11 deletions lucene/core/src/java/org/apache/lucene/index/OrdinalMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,12 @@ public static OrdinalMap build(IndexReader.CacheKey owner, TermsEnum subs[], lon

/** Cache key of whoever asked for this awful thing */
public final IndexReader.CacheKey owner;
// number of global ordinals
final long valueCount;
// globalOrd -> (globalOrd - segmentOrd) where segmentOrd is the the ordinal in the first segment that contains this term
final PackedLongValues globalOrdDeltas;
final LongValues globalOrdDeltas;
// globalOrd -> first segment container
final PackedLongValues firstSegments;
final LongValues firstSegments;
// for every segment, segmentOrd -> globalOrd
final LongValues segmentToGlobalOrds[];
// the map from/to segment ids
Expand Down Expand Up @@ -271,13 +273,26 @@ protected boolean lessThan(TermsEnumIndex a, TermsEnumIndex b) {
globalOrd++;
}

this.firstSegments = firstSegments.build();
this.globalOrdDeltas = globalOrdDeltas.build();
long ramBytesUsed = BASE_RAM_BYTES_USED + segmentMap.ramBytesUsed();
this.valueCount = globalOrd;

// If the first segment contains all of the global ords, then we can apply a small optimization
// and hardcode the first segments and global ord deltas as all zeroes.
Copy link
Member

Choose a reason for hiding this comment

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

Insert possessive quote (first segment's)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think the possessive quote gives the right meaning? Perhaps I could say 'first segment indices' here to be more clear.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I thought it was the first segment's deltas as all zeros and alos the global ord deltas as all zeros? But I'm OK with just rewording it to make it less controversial, or even just leaving this wording!

if (ordDeltaBits.length > 0 && ordDeltaBits[0] == 0L && ordDeltas[0].size() == this.valueCount) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm why only the first segment? Couldn't it be the 3rd segment, in addition, that matches the global ords?

Edit: ahh OK I understand now -- this opto is indeed specific to the first segment, so we can store this.firstSegments as all 0s. Good!

Do we (somewhere, couldn't find it here) pre-sort all segments by the cardinality descending? Then we could know all segments that meet this optimization are at the start of the segments list, and possibly building the ordinal map is faster (not sure). But then we would need to un-sort in the end to return the final OrdinalMap. But it might enable this opto to apply more often, except, I think we would then need an additional dereference on lookup, hmm.

Does our PackedLongValues.monotonicBuilder already optimize for the case where it is all 0s, for the case where another segment (not the first) has all the global values as well?

Copy link
Member Author

@jtibshirani jtibshirani Oct 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do we (somewhere, couldn't find it here) pre-sort all segments by the cardinality descending?

We do in fact -- the segments are sorted by 'weight', which in all call sites corresponds to the number of unique terms. This was added in LUCENE-5782.

Does our PackedLongValues.monotonicBuilder already optimize for the case where it is all 0s, for the case where another segment (not the first) has all the global values as well?

When constructing the individual PackedInts.Reader instances, we do identify the all 0s case and use the lightweight PackedInts.NullReader. It's great we optimize that case, but it does mean this PR doesn't make an enormous space difference.

Copy link
Member

Choose a reason for hiding this comment

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

Do we (somewhere, couldn't find it here) pre-sort all segments by the cardinality descending?

We do in fact -- the segments are sorted by 'weight', which in all call sites corresponds to the number of unique terms. This was added in LUCENE-5782.

Ahh, so then we know the first segment will indeed have the most unique terms, and therefore the highest chance of having "all 0s" ord deltas.

I think 2nd and 3rd segments also might have all 0s ord deltas? But we can try to optimize that in a followon issue ... progress not perfection!

Does our PackedLongValues.monotonicBuilder already optimize for the case where it is all 0s, for the case where another segment (not the first) has all the global values as well?

When constructing the individual PackedInts.Reader instances, we do identify the all 0s case and use the lightweight PackedInts.NullReader. It's great we optimize that case, but it does mean this PR doesn't make an enormous space difference.

Got it. Well, it's great that all these layers optimize :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think 2nd and 3rd segments also might have all 0s ord deltas? But we can try to optimize that in a followon issue ... progress not perfection!

I wonder if you are confused here, the proposed changes optimize the mapping from global ordinals to the ordinals of one arbitrary segment. When a segment has all value, we can simplify by always picking this segment, but there is no need to optimize this for the 2nd or 3rd segments, since we only need to be able to translate global ordinals to the ordinal of a single segment. Or maybe I'm the one confused by what you were suggesting. :)

Copy link
Member

Choose a reason for hiding this comment

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

Aha! Sorry, I was indeed confused ;)

This is to enable "retrieve BytesRef for this global ordinal" use-case, right? For that, we first pick a segment to use (the first one also containing that BytesRef), then map to its segment-local ordinal, then retrieve the BytesRef for that using the existing doc values API for that segment.

We do not (need to, nor) expose an API today to "retrieve segment N's ordinal corresponding to global ordinal M". Only the reverse direction (segment N's ordinal M maps to global ordinal O).

I think I understand now!

Copy link
Contributor

Choose a reason for hiding this comment

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

This is to enable "retrieve BytesRef for this global ordinal" use-case, right?

Right!

We do not (need to, nor) expose an API today to "retrieve segment N's ordinal corresponding to global ordinal M"

Correct.

this.firstSegments = LongValues.ZEROES;
this.globalOrdDeltas = LongValues.ZEROES;
ramBytesUsed += RamUsageEstimator.shallowSizeOf(LongValues.ZEROES);
Copy link
Contributor

Choose a reason for hiding this comment

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

We could ignore it completely from ramBytesUsed, since this singleton is allocated anyway, regardless of whether the optimization uses it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this to address a failure in TestOrdinalMap. But now I see it makes more sense to modify the test !

} else {
PackedLongValues packedFirstSegments = firstSegments.build();
PackedLongValues packedGlobalOrdDeltas = globalOrdDeltas.build();
this.firstSegments = packedFirstSegments;
this.globalOrdDeltas = packedGlobalOrdDeltas;
ramBytesUsed += packedFirstSegments.ramBytesUsed() + packedGlobalOrdDeltas.ramBytesUsed();
}

// ordDeltas is typically the bottleneck, so let's see what we can do to make it faster
segmentToGlobalOrds = new LongValues[subs.length];
long ramBytesUsed = BASE_RAM_BYTES_USED + this.globalOrdDeltas.ramBytesUsed()
+ this.firstSegments.ramBytesUsed() + RamUsageEstimator.shallowSizeOf(segmentToGlobalOrds)
+ segmentMap.ramBytesUsed();
ramBytesUsed += RamUsageEstimator.shallowSizeOf(segmentToGlobalOrds);
for (int i = 0; i < ordDeltas.length; ++i) {
final PackedLongValues deltas = ordDeltas[i].build();
if (ordDeltaBits[i] == 0L) {
Expand Down Expand Up @@ -317,6 +332,7 @@ public long get(long ord) {
ramBytesUsed += RamUsageEstimator.shallowSizeOf(segmentToGlobalOrds[i]);
}
}

this.ramBytesUsed = ramBytesUsed;
}

Expand Down Expand Up @@ -348,7 +364,7 @@ public int getFirstSegmentNumber(long globalOrd) {
* Returns the total number of unique terms in global ord space.
*/
public long getValueCount() {
return globalOrdDeltas.size();
return valueCount;
}

@Override
Expand All @@ -359,10 +375,9 @@ public long ramBytesUsed() {
@Override
public Collection<Accountable> getChildResources() {
List<Accountable> resources = new ArrayList<>();
resources.add(Accountables.namedAccountable("global ord deltas", globalOrdDeltas));
resources.add(Accountables.namedAccountable("first segments", firstSegments));
resources.add(Accountables.namedAccountable("segment map", segmentMap));
// TODO: would be nice to return actual child segment deltas too, but the optimizations are confusing
// TODO: would be nice to return the ordinal and segment maps too, but it's not straightforward
// because of optimizations.
Copy link
Contributor

Choose a reason for hiding this comment

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

could be do something like if (firstSegments != LongValues.ZEROES) { resources.add(Accountables.namedAccountable("first segments", firstSegments)); }?

Copy link
Member Author

@jtibshirani jtibshirani Oct 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think we'd need a cast here, since LongValues doesn't implement Accountable. Alternatively, we could consider a bigger change to have LongValues implement Accountable.

Update: I just saw LUCENE-9387, it probably doesn't make sense to increase usage of Accountable.

return resources;
}
}