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

Add target search concurrency to TieredMergePolicy #13430

Conversation

carlosdelest
Copy link
Contributor

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.

@carlosdelest carlosdelest changed the title Add minimum number of segments to TieredMergePolicy WIP - Add minimum number of segments to TieredMergePolicy May 27, 2024
Copy link
Contributor

@jpountz jpountz left a 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 least minNumSegments if there is a single tier, or minNumSegments-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 than allowedDocCount 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?

@carlosdelest
Copy link
Contributor Author

Hi @jpountz , thanks for your thoughts!

Let me check whether I got your suggestion right:

  • There are at most 2 segment tiers. The first has minNumSegments at most, the second minNumSegments-1 to ensure we maintain minNumSegments total segments at the most.
  • The max number of docs allowed in a segment is (maxDocs - numDeletes) / minNumSegments. That avoids creating larger segments than necessary so we ensure there's minNumSegments segments at a minimum.

Does it align with your thoughts?

Some questions I have:

  • segsPerTier is not taken into account if minNumSegments is specified, correct?
  • In case segments reach maxMergedSegmentBytes, should we fall back to the previous segsPerTier behaviour?

I see that minNumSegments is not a good name with this approach - I think targetSearchConcurrency makes sense to be explicit.

Thanks!

@jpountz
Copy link
Contributor

jpountz commented May 29, 2024

There are at most 2 segment tiers.

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.

segsPerTier is not taken into account if minNumSegments is specified, correct?

If minNumSegments is bigger than segsPerTier, then the highest tier should indeed allow for up to minNumSegments (or maybe minNumSegments-1 if there are multiple tiers, though it may introduce complexity for little value, maybe it's better/easier to keep things simple and allow for minNumSegments on the highest tier all the time). But lower tiers should still aim for segsPerTier segments.

In case segments reach maxMergedSegmentBytes, should we fall back to the previous segsPerTier behaviour?

I can understood this question in different ways, so I'll try to clarify how I think these two parameters should interact:

  • Even if minNumSegments is configured, we should still honor maxMergedSegmentBytes and not create segments that are bigger than that.
  • If your merge policy has segsPerTier = 10, maxMergedSegmentMB = 5GB, floorSegmentBytes = 100MB, minNumSegments = 12, and the total index size is 15GB, it should aim for allowedSegCount=23 segments (10 100MB segments ~=1GB, 14 1GB segments). So even though we could have created a segment of the maximum merged size by merging 10 1GB segments, we did not do it because then we would have ended up with a segment that had more than maxDoc/12 documents (assuming all docs contribute equally to the index size).

@mikemccand
Copy link
Member

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 targetSearchConcurrency -- it makes it clear this is about optimizing the "inter-segment concurrent search latency" in the resulting index, and, I think it also makes it clear that maxMergedSegmentBytes still wins. I think simply aiming for the largest tier to allow targetSearchConcurrency segment count when that exceeds segsPerTier is a good approach. After all it is those biggest segments that you really need for concurrency since searching them takes the most CPU / latency -- the lower tiers (1/10th, 1/100th, ... the size of the top tier) will be super fast by comparison and not the long pole in each query's latency.

@carlosdelest
Copy link
Contributor Author

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;
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

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);
Copy link
Contributor

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;
       }

Copy link
Contributor Author

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);
Copy link
Contributor

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.

Copy link
Contributor Author

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor Author

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;
Copy link
Contributor Author

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);
Copy link
Contributor Author

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);
Copy link
Contributor Author

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 {
Copy link
Contributor Author

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;
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 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.

Copy link
Contributor

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.

Copy link
Contributor Author

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
Copy link
Contributor

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.

Suggested change
if (segCountLevel < maxNumSegmentsOnHighestTier
if (segCountLevel <= maxNumSegmentsOnHighestTier

Copy link
Contributor Author

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());
Copy link
Contributor

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?

Copy link
Contributor Author

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;
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, applied the change.

@jpountz
Copy link
Contributor

jpountz commented Jun 24, 2024

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 TieredMergePolicy to perform sub-optimal merging.

I tried to play with other approaches, and one that seems to perform closer to what I would expect consists of removing the targetConcurrency biggest segments from the list of eligible segments, in a similar way to how we remove too big segments. And then let the merge policy do merging on the long tail of smaller segments.

@jpountz
Copy link
Contributor

jpountz commented Jun 24, 2024

@carlosdelest I took the freedom of pushing some changes to your fork, I hope you don't mind!

@jpountz jpountz marked this pull request as ready for review June 24, 2024 16:34
@jpountz jpountz changed the title WIP - Add minimum number of segments to TieredMergePolicy Add target search concurrency to TieredMergePolicy Jun 24, 2024
@jpountz jpountz added this to the 9.12.0 milestone Jun 24, 2024
@jpountz
Copy link
Contributor

jpountz commented Jun 24, 2024

FWIW I have been playing with BaseMergePolicyTestCase's doTestSimulateAppendOnly and doTestSimulateUpdates. With this change, I'm observing a number of segments in the order of targetSearchConcurrency on the highest tier while increasing the segment count by a bit less targetSearchConcurrency on average and without affecting the write amplification significantly.

jpountz added a commit to jpountz/lucene that referenced this pull request Jun 25, 2024
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.
@carlosdelest
Copy link
Contributor Author

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 testSimulateUpdates()

Is there any other work / checks we should be doing for this?

Copy link
Contributor

@jpountz jpountz left a 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?

@carlosdelest
Copy link
Contributor Author

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

if (rarely(r)) {
tmp.setTargetSearchConcurrency(TestUtil.nextInt(r, 2, 20));
} else {
tmp.setTargetSearchConcurrency(TestUtil.nextInt(r, 10, 50));
Copy link
Contributor

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?

Copy link
Contributor Author

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

@jpountz jpountz merged commit 22ca695 into apache:main Jul 17, 2024
3 checks passed
goankur pushed a commit to goankur/lucene that referenced this pull request Jul 18, 2024
jpountz added a commit that referenced this pull request Jul 18, 2024
This adds the same `targetSearchConcurrency` parameter to `LogMergePolicy`
that #13430 is adding to `TieredMergePolicy`.
jpountz added a commit that referenced this pull request Jul 18, 2024
This adds the same `targetSearchConcurrency` parameter to `LogMergePolicy`
that #13430 is adding to `TieredMergePolicy`.
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.

Add a MergePolicy wrapper that preserves search concurrency?
3 participants