-
Notifications
You must be signed in to change notification settings - Fork 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
Reduce FST block size for BlockTreeTermsWriter #12604
Conversation
Here is the young GC statistics and allocation profile after indexing
Baseline Allocation Profile
Candidate Allocation Profile
|
Hi @jpountz ! Would you please take a look at this PR when you have time? Looking forward to getting your suggestions on this topic ~ |
Oh, interesting find, it makes sense to me but I'm not the most familiar one with this piece of code. @mikemccand or @s1monw what do you think? |
This change makes sense to me. @mikemccand WDYT |
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.
The only possible risk I see is that when writing truly massive segments with many terms in a given field, a large FST will now require 32X the number of blocks to hold it. This larger array will add GC cost, slow down writing a bit, etc.
But I think that's an OK tradeoff: writing such large segments is already massively more costly than writing tiny segments, so in proportion that added cost is acceptable.
We are not really at risk of exhausting the max size of a java array -- that'd happen if one tried to build a ~2 TB FST, which is not even really feasible today since the FST is still fully heap resident at write time.
Thanks @gf2121!
lucene/CHANGES.txt
Outdated
@@ -163,6 +163,8 @@ Optimizations | |||
* GITHUB#12382: Faster top-level conjunctions on term queries when sorting by | |||
descending score. (Adrien Grand) | |||
|
|||
* GITHUB#12604: Reduce block size of FST BytesStore in BlockTreeTermsWriter. (Guo Feng) |
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.
Could you add some words that the end user could understand -- maybe reducing GC load during indexing
or so?
@mikemccand maybe we can tradeoff here between segments we write the first time ie through IW and segments we write caused by a merge? it might mitigate your concerns. |
Thanks for all review and suggestions here!
Thanks @s1monw , I really like the idea that we can estimate the page size before building the FST! A tiny concern is that we could probably build a big FST if IW has a large flush buffer, or we could build small FST when tiny segments merge. This commit tries a way to estimate a more accurate size of the FST. I did the similar count of
|
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 like this approach! I had forgotten that BlockTree
creates little baby FSTs all the way up the prefix tree, so it's creating (and discarding) many FSTs in order to build the final FST, making this change all the more important!
I just left a tiny comment about the CHANGES
entry and a few "later" comments -- maybe open follow-on issues for those ideas?
lucene/CHANGES.txt
Outdated
@@ -163,6 +163,9 @@ Optimizations | |||
* GITHUB#12382: Faster top-level conjunctions on term queries when sorting by | |||
descending score. (Adrien Grand) | |||
|
|||
* GITHUB#12604: Estimate the block size of FST BytesStore in BlockTreeTermsWriter | |||
to reducing GC load during indexing. (Guo Feng) |
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.
reducing -> reduce
@@ -490,10 +491,22 @@ public void compileIndex( | |||
} | |||
} | |||
|
|||
long estimateSize = prefix.length; |
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.
Too bad we don't have a writer that uses tiny (like 8 bytes) block at first, but doubles size for each new block (16 bytes, 32 bytes next, etc.). Then we would naturally use log(size) number of blocks without over-allocating.
But then reading bytes is a bit tricky because we'd need to take discrete log (base 2) of the address. Maybe it wouldn't be so bad -- we could do this with Long.numberOfLeadingZeros
maybe? But that's a bigger change ... we can do this separately/later.
@@ -490,10 +491,22 @@ public void compileIndex( | |||
} | |||
} | |||
|
|||
long estimateSize = prefix.length; | |||
for (PendingBlock block : blocks) { | |||
if (block.subIndices != null) { |
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 also should really explore the TODO
above to write vLong
in opposite byte order -- this might save quite a bit of storage in the FST since outputs would share more prefixes. Again, separate issue 😀
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.
LGTM too
It looks like there's a bit less Young GC in nightly benchmarks since this change was merged, from 6-8 seconds, to consistently below 6s. I pushed an annotation. |
@jpountz Thanks for annotating ! I also checked
Before Patch https://blunders.io/jfr-demo/indexing-4kb-2023.10.03.18.03.47/allocations-drill-down |
Description
https://blunders.io/jfr-demo/indexing-4kb-2023.09.25.18.03.36/allocations-drill-down
Nightly benchmark shows that
FSTCompiler#init
allocated most of the memory during indexing. This is becauseFSTCompiler#init
will always allocate 32k bytes as we parambytesPageBits
default to 15. I counted the usage of BytesStore (getPosition()
whenBytesStore#finish
called) during the wikimediumall indexing, and the result shows that 99% FST won't even use more than 1k bytes.This PR proposes to reduce the block size of
FST
inLucene90BlockTreeTermsWriter
.closes #12598