Completion FSTs to be loaded off-heap at all times #14364

javanna · 2025-03-17T15:30:12Z

All the existing completion postings format load their FSTs on-heap. It is possible to customize that behaviour by mainintaing a custom postings format that override the fst load mode.

TestSuggestField attempted to test all modes, but in practice, it can only test the default load mode because the read path relies on the default constructor called via SPI, that does not allow providing the fst load mode.

This commit switches loading of FST to off-heap, and removes the fst load mode argument from all completion postings format classes, effectively making the loading always off-heap.

See related discussion where this change originated at #14275 .

All the existing completion postings format load their FSTs on-heap. It is possible to customize that behaviour by mainintaing a custom postings format that override the fst load mode. TestSuggestField attempted to test all modes, but in practice, it can only test the default load mode because the read path relies on the default constructor called via SPI, that does not allow providing the fst load mode. This commit changes the default value to OFF_HEAP, and addresses the testing gap in TestSuggestField, so that all load modes are tested. This unveiled a bug when loading FST off heap and the index input is a MemorySegmentIndexInput which is fixed by apache#14271.

javanna · 2025-03-17T18:53:37Z

.../suggest/src/java/org/apache/lucene/search/suggest/document/Completion101PostingsFormat.java

-   * Creates a {@link Completion101PostingsFormat} that will use the provided <code>fstLoadMode
-   * </code> to determine if the completion FST should be loaded on or off heap.
-   */
-  public Completion101PostingsFormat(FSTLoadMode fstLoadMode) {


I find that these constructors taking the fst load mode are trappy, in that they make readers think that calling them allows to override the load mode. In practice, the load mode is only ever used at read time, and that codepath goes through SPI loading, so via the default constructor. I think removing these is the best path forward to avoid confusion.

javanna · 2025-03-17T18:53:55Z

lucene/suggest/src/java/org/apache/lucene/search/suggest/document/CompletionPostingsFormat.java

@@ -122,11 +122,6 @@ public enum FSTLoadMode {

  private final FSTLoadMode fstLoadMode;

-  /** Used only by core Lucene at read-time via Service Provider instantiation */
-  public CompletionPostingsFormat(String name) {


This was unused and redundant.

jpountz · 2025-03-17T21:42:00Z

lucene/suggest/src/test/org/apache/lucene/search/suggest/document/TestSuggestField.java

+                protected PostingsFormat delegatePostingsFormat() {
+                  return delegate.delegatePostingsFormat();
+                }
+              };


FWIW this may work accidentally on NRT segments if IndexWriter opens them with the codec configured on the IndexWriterConfig, but it you were to open an index from a Directory using this postings format, it would open it using the default FST load mode and ignore the one passed to the constructor here. This is because, Lucene would read "Completion101" from the segment infos and look it up through SPI, which would return the default format that is registered under this name rather than this instance that you are creating here.

I wonder if we should remove FSTLoadMode completely. To your point, it's trappy to think that this thing is actually configurable?

you are right, I tried to quickly hack this together and I ended up making the same mistake we already had in existing tests. Shame on me! I really do need to register the custom postings format to SPI for this to leverage the random fst load mode. I was also thinking about the option to remove configurability of fst load mode. That would certainly simplify things and avoid this same mistake in the future. Either that, or have a proper example of how the postings format load mode can be customized.

I updated this PR to remove the option to configure the load mode entirely.

jpountz

LGTM, don't miss adding a CHANGES entry. :) We may want to add back-compat for NRTSuggester#load(IndexInput, FstLoadMode) upon backporting?

javanna · 2025-03-19T09:13:13Z

Thanks @jpountz what's your suggestion around back-compat? Sounds like you are suggesting not backporting the removal of the fst load mode enum but only the switch to off-heap by default, is that accurate?

jpountz · 2025-03-19T09:20:27Z

I was thinking of keeping the load(IndexInput, FSTLoadMode) static method, documenting that the load mode is ignored and deprecating it. Indeed that would require keeping the FSTLoadMode enum, which would be deprecated too.

javanna · 2025-03-19T11:20:04Z

Cool then I will target this PR at main only, and open a separate PR for branch_10x. Out of curiosity, what are the usecases where you'd expect users to call NRTSuggester#load directly?

jpountz · 2025-03-19T12:31:56Z

Good question. The class is public and looked like a user-facing API hence my comment, but you can't serialize a NRTSuggester yourself since NRTSuggesterBuilder is pkg-private. So it looks like this load method should really be pkg-private too?

javanna · 2025-03-19T13:26:59Z

I opened #14372 to address the visibility issue of load, that should simplify this PR and backporting it once merged.

javanna · 2025-03-19T18:59:36Z

I merged main in after merging #14372 and added a changelog entry. I believe this is ready to go, and can now be backported to branch_10x as-is.

A while ago we introduced a completion postings format extension to eventually be able to customize how completion FSTs are loaded. See elastic#111494. We have never leveraged this extension, and meanwhile Lucene is moving to always load FSTs off-heap, and no longer allow on-heap. See apache/lucene#14364 . This commit removes the SPI extension as it is no longer needed.

jpountz · 2025-03-19T21:43:47Z

Agreed!

javanna · 2025-03-19T22:08:54Z

Thanks @jpountz for all the help!

All the existing completion postings format load their FSTs on-heap. It is possible to customize that behaviour by mainintaing a custom postings format that override the fst load mode. TestSuggestField attempted to test all modes, but in practice, it can only test the default load mode because the read path relies on the default constructor called via SPI, that does not allow providing the fst load mode. This commit switches loading of FST to off-heap, and removes the fst load mode argument from all completion postings format classes, effectively making the loading always off-heap.

A while ago we introduced a completion postings format extension to eventually be able to customize how completion FSTs are loaded. See #111494. We have never leveraged this extension, and meanwhile Lucene is moving to always load FSTs off-heap, and no longer allow on-heap. See apache/lucene#14364 . This commit removes the SPI extension as it is no longer needed.

A while ago we introduced a completion postings format extension to eventually be able to customize how completion FSTs are loaded. See elastic#111494. We have never leveraged this extension, and meanwhile Lucene is moving to always load FSTs off-heap, and no longer allow on-heap. See apache/lucene#14364 . This commit removes the SPI extension as it is no longer needed.

All the existing completion postings format load their FSTs on-heap. It is possible to customize that behaviour by mainintaing a custom postings format that override the fst load mode. TestSuggestField attempted to test all modes, but in practice, it can only test the default load mode because the read path relies on the default constructor called via SPI, that does not allow providing the fst load mode. This commit switches loading of FST to off-heap, and removes the fst load mode argument from all completion postings format classes, effectively making the loading always off-heap.

A while ago we introduced a completion postings format extension to eventually be able to customize how completion FSTs are loaded. See elastic#111494. We have never leveraged this extension, and meanwhile Lucene is moving to always load FSTs off-heap, and no longer allow on-heap. See apache/lucene#14364 . This commit removes the SPI extension as it is no longer needed.

github-project-automation bot added this to OpenSearch Lucene & Core Performance Tracking Mar 17, 2025

github-project-automation bot moved this to Open in OpenSearch Lucene & Core Performance Tracking Mar 17, 2025

github-actions bot added the module:suggest label Mar 17, 2025

This was referenced Mar 17, 2025

Address completion fields testing gap and truly allow loading FST off heap #14270

Closed

Support load per-iteration replacement of NamedSPI #14275

Open

javanna commented Mar 17, 2025

View reviewed changes

iter

085aa9a

jpountz reviewed Mar 17, 2025

View reviewed changes

iter

b21f1ee

javanna changed the title ~~Completion FSTs to be loaded off-heap by default~~ Completion FSTs to be loaded off-heap at all times Mar 18, 2025

javanna added 3 commits March 18, 2025 13:05

iter

289d475

iter

fe0b899

iter

fc8b6f1

jpountz approved these changes Mar 18, 2025

View reviewed changes

javanna added 2 commits March 19, 2025 19:54

Merge branch 'main' into enhancement/completion_fst_off_heap

48455da

iter

5a45a0a

javanna added this to the 10.2.0 milestone Mar 19, 2025

javanna mentioned this pull request Mar 19, 2025

Remove completion postings format extension elastic/elasticsearch#125253

Merged

javanna merged commit fe86094 into apache:main Mar 19, 2025
7 checks passed

github-project-automation bot moved this from Open to Merged in OpenSearch Lucene & Core Performance Tracking Mar 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Completion FSTs to be loaded off-heap at all times #14364

Completion FSTs to be loaded off-heap at all times #14364

javanna commented Mar 17, 2025 •

edited

Loading

javanna Mar 17, 2025

javanna Mar 17, 2025

jpountz Mar 17, 2025

javanna Mar 18, 2025

javanna Mar 18, 2025

jpountz left a comment

javanna commented Mar 19, 2025

jpountz commented Mar 19, 2025

javanna commented Mar 19, 2025

jpountz commented Mar 19, 2025

javanna commented Mar 19, 2025

javanna commented Mar 19, 2025

jpountz commented Mar 19, 2025

javanna commented Mar 19, 2025

Completion FSTs to be loaded off-heap at all times #14364

Completion FSTs to be loaded off-heap at all times #14364

Conversation

javanna commented Mar 17, 2025 • edited Loading

javanna Mar 17, 2025

Choose a reason for hiding this comment

javanna Mar 17, 2025

Choose a reason for hiding this comment

jpountz Mar 17, 2025

Choose a reason for hiding this comment

javanna Mar 18, 2025

Choose a reason for hiding this comment

javanna Mar 18, 2025

Choose a reason for hiding this comment

jpountz left a comment

Choose a reason for hiding this comment

javanna commented Mar 19, 2025

jpountz commented Mar 19, 2025

javanna commented Mar 19, 2025

jpountz commented Mar 19, 2025

javanna commented Mar 19, 2025

javanna commented Mar 19, 2025

jpountz commented Mar 19, 2025

javanna commented Mar 19, 2025

javanna commented Mar 17, 2025 •

edited

Loading