-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add target search concurrency to TieredMergePolicy #13430
Add target search concurrency to TieredMergePolicy #13430
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into it!
Currently, it seems to support this new option the same way that it handles the max merged segment size, ie. by greedily merging segments together if the merged segment can be just as large as the target max merged segment size?
Intuitively, this doesn't feel right to me as this boils down to merging more aggressively when minNumSegments
is configured, while I'd expect the opposite: less aggressive merging in order to preserve search concurrency. I'm curious to get thoughts on a different approach that would consist of:
- Update the calculation of
allowedSegCount
to take this new option into account, by allowing at leastminNumSegments
if there is a single tier, orminNumSegments-1
segments on the highest tier otherwise (since the previous tier would have as many docs as a segment on the highest tier). - Update
doFindMerge
to simply ignore candidate merges that have more thanallowedDocCount
docs.
I'm curious to get @mikemccand 's thoughts.
Separately, I don't think the minNumSegments
is good. This suggests that the merge policy will ensure the index has at least this number of segments, which is not what it is doing. Maybe call in targetSearchConcurrenty
or targetNumSlices
instead to better reflect the connection with search?
Hi @jpountz , thanks for your thoughts! Let me check whether I got your suggestion right:
Does it align with your thoughts? Some questions I have:
I see that Thanks! |
Well, there can be more tiers, but since tiers have exponential sizes (e.g. if you merge factor is 10, each tier has segments that are 10x bigger than the previous tier) it's almost certainly fine to ignore segments on tiers beyond the 2nd higher tier, they would account to very few documents compared with the first 2 tiers.
If
I can understood this question in different ways, so I'll try to clarify how I think these two parameters should interact:
|
Thank you for tackling this @carlosdelest! What a hairy challenge ... TMP really is its own little baby chess engine, with many things it is trying to optimize towards (what the ML world now calls "multi-objective"), playing against a worthy opponent (the indexing application that keeps indexing different sized docs, doing some deletions/updates, refresh/commit, etc., producing all kinds of exotic segments). And of course in addition to the app exotically birthing new segments, TMP itself is causing new big segments to appear when each merge completes! This really is a game theory problem, heh. +1 for the name |
Thanks @mikemccand and @jpountz for your explanations! This is much clearer now. I've updated the PR, LMKWYT about the approach! I'm looking into effective ways of testing in terms of ensuring that the last tier contains the target number of segments for search concurrency. Any suggestions on how can I check that the last tier eventually contains the expected number of segments? |
@@ -93,6 +93,7 @@ public class TieredMergePolicy extends MergePolicy { | |||
private double segsPerTier = 10.0; | |||
private double forceMergeDeletesPctAllowed = 10.0; | |||
private double deletesPctAllowed = 20.0; | |||
private int targetSearchConcurrency = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it default to 1? I expect it to yield the same behavior as today?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - I iterated a bit on this but now it's equivalent. Thanks!
@@ -257,6 +258,25 @@ public double getSegmentsPerTier() { | |||
return segsPerTier; | |||
} | |||
|
|||
/** | |||
* Sets the target search concurrency. This allows merging to ensure that there are at least | |||
* targetSearchConcurrency segments on the top tier. This setting can be overriden by force |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rather say that this prevents creating segments that are bigger than maxDoc/targetSearchConcurrency, which in-turn makes the work parallelizable into targetSearchConcurrency
slices of similar doc counts?
I think it's also worth clarifying that this makes merging less aggressive, ie. high values of targetSearchConcurrency
result in indexes that do less merging and have more segments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, updating.
@@ -409,6 +433,8 @@ public MergeSpecification findMerges( | |||
// allowedSegCount may occasionally be less than segsPerTier | |||
// if segment sizes are below the floor size | |||
allowedSegCount = Math.max(allowedSegCount, segsPerTier); | |||
// Override with the targetSearchConcurrency if it is greater | |||
allowedSegCount = Math.max(allowedSegCount, targetSearchConcurrency); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also do a change like this one a few lines above:
diff --git a/lucene/core/src/java/org/apache/lucene/index/TieredMergePolicy.java b/lucene/core/src/java/org/apache/lucene/index/TieredMergePolicy.java
index 208aa297287..4126601e51e 100644
--- a/lucene/core/src/java/org/apache/lucene/index/TieredMergePolicy.java
+++ b/lucene/core/src/java/org/apache/lucene/index/TieredMergePolicy.java
@@ -392,13 +392,14 @@ public class TieredMergePolicy extends MergePolicy {
allowedDelCount = Math.max(0, allowedDelCount);
final int mergeFactor = (int) Math.min(maxMergeAtOnce, segsPerTier);
+ final int maxNumSegmentsOnHighestTier = Math.max(segsPerTier, targetSearchConcurrency);
// Compute max allowed segments in the index
long levelSize = Math.max(minSegmentBytes, floorSegmentBytes);
long bytesLeft = totIndexBytes;
double allowedSegCount = 0;
while (true) {
final double segCountLevel = bytesLeft / (double) levelSize;
- if (segCountLevel < segsPerTier || levelSize == maxMergedSegmentBytes) {
+ if (segCountLevel <= maxNumSegmentsOnHighestTier || levelSize == maxMergedSegmentBytes) {
allowedSegCount += Math.ceil(segCountLevel);
break;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, now the highest tier needs to take that into account. Thanks! 👍
break; | ||
} | ||
|
||
final MergeScore score = score(candidate, hitTooLarge, segInfosSizes); | ||
final MergeScore score = score(candidate, hitTooLarge || hitMaxDocs, segInfosSizes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that we want to do this. hitTooLarge
is considered a good thing by this merge policy. It means that we found a merge that reached the maximum merged segment size. It's great because if we perform this merge, then this segment will not be eligible again for merging (unless it gets many deletes), so we'll essentially be done with it. So these merges are given a very good score by assuming that they have a perfect skew.
hitMaxDocs
is different, we don't want to prioritize unbalanced merges that produce segments of hitMaxDocs
documents? It's better to keep prioritizing balanced small segment merges as usual?
My expectation for handling allowedMaxDoc
is that we would just never score any merge that has more than allowedMaxDoc
documents and force the merge policy to select one of the candidate merges that produce fewer documents than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - I was thinking in doing the same with docs than with bytes, but I see they're quite different. Docs is more a hint to maintain the number of segments we want, but bytes provides a hard limit on size and also score guidance on segments to merge.
@@ -431,6 +447,128 @@ public void testForcedMergesRespectSegSize() throws Exception { | |||
dir.close(); | |||
} | |||
|
|||
public void testForcedMergesRespectsTargetSearchConcurrency() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to check out BaseMergePolicyTestCase#testSimulateAppendOnly
. It simulates merging without actually indexing docs, which allows simulating many many rounds of merging in very little time, while comparing how some parameters (like targetSearchConcurrency
) affect merging decisions and metrics such as write amplification through merges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've given it a try - LMKWYT!
@@ -257,6 +258,25 @@ public double getSegmentsPerTier() { | |||
return segsPerTier; | |||
} | |||
|
|||
/** | |||
* Sets the target search concurrency. This allows merging to ensure that there are at least | |||
* targetSearchConcurrency segments on the top tier. This setting can be overriden by force |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, updating.
@@ -93,6 +93,7 @@ public class TieredMergePolicy extends MergePolicy { | |||
private double segsPerTier = 10.0; | |||
private double forceMergeDeletesPctAllowed = 10.0; | |||
private double deletesPctAllowed = 20.0; | |||
private int targetSearchConcurrency = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes - I iterated a bit on this but now it's equivalent. Thanks!
@@ -409,6 +433,8 @@ public MergeSpecification findMerges( | |||
// allowedSegCount may occasionally be less than segsPerTier | |||
// if segment sizes are below the floor size | |||
allowedSegCount = Math.max(allowedSegCount, segsPerTier); | |||
// Override with the targetSearchConcurrency if it is greater | |||
allowedSegCount = Math.max(allowedSegCount, targetSearchConcurrency); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, now the highest tier needs to take that into account. Thanks! 👍
break; | ||
} | ||
|
||
final MergeScore score = score(candidate, hitTooLarge, segInfosSizes); | ||
final MergeScore score = score(candidate, hitTooLarge || hitMaxDocs, segInfosSizes); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - I was thinking in doing the same with docs than with bytes, but I see they're quite different. Docs is more a hint to maintain the number of segments we want, but bytes provides a hard limit on size and also score guidance on segments to merge.
@@ -431,6 +447,128 @@ public void testForcedMergesRespectSegSize() throws Exception { | |||
dir.close(); | |||
} | |||
|
|||
public void testForcedMergesRespectsTargetSearchConcurrency() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've given it a try - LMKWYT!
int segDocCount = segSizeDocs.maxDoc - segSizeDocs.delCount; | ||
if (docCountThisMerge + segDocCount > allowedDocCount) { | ||
// We don't want to merge segments that will produce more documents than allowedDocCount | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should break
here, not continue
. continue
allows producing merges of segments whose sizes are not adjacent, I don't think we should allow this for the doc count condition, as this potentially makes merging run in quadratic time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably also should produce a singleton merge here in case candidate.isEmpty()
(see what we're doing for too large segments below). Most likely this suggested merge will not be selected because it will have a low score, though it could end up selected if it reclaims lots of deletes and we don't have better merges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks!
// below we make the assumption that segments that reached the max segment | ||
// size divided by 2 don't need merging anymore | ||
int mergeFactor = (int) Math.min(tmp.getSegmentsPerTier(), tmp.getMaxMergeAtOnce()); | ||
while (true) { | ||
final double segCountLevel = bytesLeft / (double) levelSizeBytes; | ||
if (segCountLevel < tmp.getSegmentsPerTier() || levelSizeBytes >= maxMergedSegmentBytes / 2) { | ||
if (segCountLevel < maxNumSegmentsOnHighestTier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it allow maxNumSegmentsOnHighestTier
for consistency with the code under main
? Though I don't expect it to make a big difference since segCountLevel
is a double.
if (segCountLevel < maxNumSegmentsOnHighestTier | |
if (segCountLevel <= maxNumSegmentsOnHighestTier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch - updating
@@ -111,11 +114,29 @@ protected void assertSegmentInfos(MergePolicy policy, SegmentInfos infos) throws | |||
} | |||
} | |||
|
|||
// There can be more segments if we can't merge docs - they are balanced between segments | |||
int maxDocsPerSegment = tmp.getMaxAllowedDocs(infos.totalMaxDoc()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we subtract deletions to totalMaxDoc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should. Thanks!
} | ||
numEligible++; | ||
} | ||
boolean eligibleDocsMerge = numEligible > 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the condition can be made simpler. Would it be ok to check the sum of the doc counts of the two smallest segments? If it's greater than the allowed doc count, then no merging is allowed, otherwise this 2-segments merge would have been a legal merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, applied the change.
I spent some time digging into the new behavior introduced by this PR, as I'd like to get it over the finish line. :) I am getting disappointing results for the write amplification. This seems to be due to the fact that we compute the segment count as if segments could magically be close to the doc count limit, but in practice this is not the case, and it forces I tried to play with other approaches, and one that seems to perform closer to what I would expect consists of removing the |
@carlosdelest I took the freedom of pushing some changes to your fork, I hope you don't mind! |
FWIW I have been playing with |
This adds the same `targetSearchConcurrency` parameter to `LogMergePolicy` that apache#13430 is adding to `TieredMergePolicy`. The implementation is simpler here since `LogMergePolicy` is constrained to only merging adjacent segments. The downside is that this option results is significantly more write amplification on this merge policy compared with `TieredMergePolicy`, which is allowed to merge non-adjacent segments. From simulating merging on an index that gets appended 555 equal-size segments (like nightly benchmarks) and a `targetSearchConcurrency` of 8, I get the following results: - 19.1 segments in the index on average, vs. 11.1 with a `targetSearchConcurrency` of 1. - Maximum number of segments in the index of 28, vs. 22 with a `targetSearchConcurrency` of 1. - Write amplification through merging of 3.6 instead of 2.9. For comparison, 3.6 is about the write amplification that you get with a merge factor of 7 otherwise. - The resulting index has 24 segments, including 11 segments on the highest tier, vs. 15 segments and 5 segments on the highest tier with a `targetSearchConcurrency` of 1.
Thanks for taking a look into this @jpountz ! Your approach makes sense to me, and addresses some of the problems I was having with bigger segments not being merged because of the number of documents, and smaller segments being discarded. The test checks makes sense as well. I've added setting target search concurrency for Is there any other work / checks we should be doing for this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just reviewed the change, it looks good to me. I also ran more simulations for write amplification, and the results look good to me:
Scenario 1: 555 flushes of exactly 60065-document segments (like nightlies) where each doc contributes 5kB to the index. No updates/deletions.
Merge policy | Write amplification | Average number of segments | Max number of segments |
---|---|---|---|
TieredMergePolicy defaults | 1.99 | 33.62 | 65 |
TieredMergePolicy targetSearchConcurrency=4 | 2.05 | 33.53 | 65 |
TieredMergePolicy targetSearchConcurrency=8 | 2.19 | 34.31 | 65 |
TieredMergePolicy targetSearchConcurrency=16 | 2.47 | 38.56 | 67 |
Scenario 2: 1000 flushes of random(1, 50k)-document segments where each doc contributes 5kB to the index. No updates/deletions.
Merge policy | Write amplification | Average number of segments | Max number of segments |
---|---|---|---|
TieredMergePolicy defaults | 2.86 | 34.6 | 54 |
TieredMergePolicy targetSearchConcurrency=4 | 2.93 | 36.0 | 56 |
TieredMergePolicy targetSearchConcurrency=8 | 2.98 | 38.8 | 58 |
TieredMergePolicy targetSearchConcurrency=16 | 3.24 | 43.6 | 61 |
Scenario 3: 1000 flushes of random(1, 50k)-document segments where each doc contributes 5kB to the index. 50% of the documents in every flush are updates, ie. each flush contributes N/2 new documents and N/2 updated documents.
Merge policy | Write amplification | Average number of segments | Max number of segments |
---|---|---|---|
TieredMergePolicy defaults | 3.13 | 28.8 | 46 |
TieredMergePolicy targetSearchConcurrency=4 | 3.12 | 28.9 | 46 |
TieredMergePolicy targetSearchConcurrency=8 | 3.31 | 30.7 | 46 |
TieredMergePolicy targetSearchConcurrency=16 | 3.66 | 33.5 | 51 |
One thing worth noting is that it becomes easier to enforce the target search concurrency as more segments get added to the index
@mikemccand would you like to take a look before I merge?
As a follow-up, I think we should enable this feature by default? Maybe only in Lucene 10? With a value such as 8?
I just pushed a minor improvement that ignores the limit on the doc count if we're still under the floor segment size, as it felt silly to apply this constraint in this case.
Is there any other work / checks we should be doing for this?
I would suggest improving LuceneTestCase#newTieredMergePolicy(Random)
should randomly set the target concurrency on the merge policy. And remove the call to the setter to the BaseMergePolicyTestCase#testSimulate*
test cases?
Done in 2d80315 |
… in append only tests
if (rarely(r)) { | ||
tmp.setTargetSearchConcurrency(TestUtil.nextInt(r, 2, 20)); | ||
} else { | ||
tmp.setTargetSearchConcurrency(TestUtil.nextInt(r, 10, 50)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected the opposite: that we commonly set a reasonable target search concurrency, and rarely set a high value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - I was mimicking the segments per tier approach here, but I guess that makes sense. Changing it
This adds the same `targetSearchConcurrency` parameter to `LogMergePolicy` that #13430 is adding to `TieredMergePolicy`.
This adds the same `targetSearchConcurrency` parameter to `LogMergePolicy` that #13430 is adding to `TieredMergePolicy`.
Closes #12877
Adds a minimum number of segments to
TieredMergePolicy
. This allows to ensure that a minimum search concurrency parallelism is achievable using inter-segment parallelism.