You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "Tony-X (via GitHub)" <gi...@apache.org> on 2023/09/12 19:20:30 UTC
[GitHub] [lucene] Tony-X opened a new pull request, #12552: Make FSTPostingsFormat load FSTs off-heap
Tony-X opened a new pull request, #12552:
URL: https://github.com/apache/lucene/pull/12552
### Description
FSTs supports to load offheap for a while. As we were trying to use `FSTPostingsFormat` for some fields we realized heap usage bumped.
Upon further investigation we realized the FSTPostingsFormat does not load FSTs offheap. This PR addresses that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] msokolov commented on a diff in pull request #12552: Make FSTPostingsFormat load FSTs off-heap
Posted by "msokolov (via GitHub)" <gi...@apache.org>.
msokolov commented on code in PR #12552:
URL: https://github.com/apache/lucene/pull/12552#discussion_r1323494538
##########
lucene/codecs/src/java/org/apache/lucene/codecs/memory/FSTTermsReader.java:
##########
@@ -191,7 +193,9 @@ final class TermsReader extends Terms {
this.sumTotalTermFreq = sumTotalTermFreq;
this.sumDocFreq = sumDocFreq;
this.docCount = docCount;
- this.dict = new FST<>(in, in, new FSTTermOutputs(fieldInfo));
+ OffHeapFSTStore offHeapFSTStore = new OffHeapFSTStore();
+ this.dict = new FST<>(in, in, new FSTTermOutputs(fieldInfo), offHeapFSTStore);
+ in.skipBytes(offHeapFSTStore.size());
Review Comment:
just curious - why do we need to skip to the end?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] Tony-X commented on a diff in pull request #12552: Make FSTPostingsFormat load FSTs off-heap
Posted by "Tony-X (via GitHub)" <gi...@apache.org>.
Tony-X commented on code in PR #12552:
URL: https://github.com/apache/lucene/pull/12552#discussion_r1323531587
##########
lucene/codecs/src/java/org/apache/lucene/codecs/memory/FSTTermsReader.java:
##########
@@ -191,7 +193,9 @@ final class TermsReader extends Terms {
this.sumTotalTermFreq = sumTotalTermFreq;
this.sumDocFreq = sumDocFreq;
this.docCount = docCount;
- this.dict = new FST<>(in, in, new FSTTermOutputs(fieldInfo));
+ OffHeapFSTStore offHeapFSTStore = new OffHeapFSTStore();
+ this.dict = new FST<>(in, in, new FSTTermOutputs(fieldInfo), offHeapFSTStore);
+ in.skipBytes(offHeapFSTStore.size());
Review Comment:
The OffHeapFSTStore doesn't advance the input (but the on-heap one does), we need to seek it manually since the input contains multiple FSTs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] msokolov merged pull request #12552: Make FSTPostingsFormat load FSTs off-heap
Posted by "msokolov (via GitHub)" <gi...@apache.org>.
msokolov merged PR #12552:
URL: https://github.com/apache/lucene/pull/12552
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] Tony-X commented on pull request #12552: Make FSTPostingsFormat load FSTs off-heap
Posted by "Tony-X (via GitHub)" <gi...@apache.org>.
Tony-X commented on PR #12552:
URL: https://github.com/apache/lucene/pull/12552#issuecomment-1726671056
Thanks @msokolov !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make FSTPostingsFormat load FSTs off-heap [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12552:
URL: https://github.com/apache/lucene/pull/12552#issuecomment-1830133688
I think this was mistakingly not backported to 9.x? (I only caught this because I was seeing merge conflicts trying to backport #12803 and saw this. I'll backport shortly -- I think this is low risk for the pending 9.9.0 release: this has baked in main for a couple months, and this is a very rarely used experimental postings format.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mikemccand commented on a diff in pull request #12552: Make FSTPostingsFormat load FSTs off-heap
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #12552:
URL: https://github.com/apache/lucene/pull/12552#discussion_r1325827523
##########
lucene/codecs/src/java/org/apache/lucene/codecs/memory/FSTTermsReader.java:
##########
@@ -191,7 +193,9 @@ final class TermsReader extends Terms {
this.sumTotalTermFreq = sumTotalTermFreq;
this.sumDocFreq = sumDocFreq;
this.docCount = docCount;
- this.dict = new FST<>(in, in, new FSTTermOutputs(fieldInfo));
+ OffHeapFSTStore offHeapFSTStore = new OffHeapFSTStore();
+ this.dict = new FST<>(in, in, new FSTTermOutputs(fieldInfo), offHeapFSTStore);
+ in.skipBytes(offHeapFSTStore.size());
Review Comment:
I was worried at first that we are not cloning this `IndexInput` anywhere and that this would cause concurrency bugs when two queries pull a `TermsEnum` here, but we are OK because `OffHeapFSTStore` does this cloning when it [pulls a random access slice from the `IndexInput`](https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/util/fst/OffHeapFSTStore.java#L66).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mikemccand commented on pull request #12552: Make FSTPostingsFormat load FSTs off-heap
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12552:
URL: https://github.com/apache/lucene/pull/12552#issuecomment-1719297920
@Tony-X have you tried passing all Lucene unit tests using this Codec? I think you can add `-Dtests.codec=...` to force all tests to use it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] msokolov commented on pull request #12552: Make FSTPostingsFormat load FSTs off-heap
Posted by "msokolov (via GitHub)" <gi...@apache.org>.
msokolov commented on PR #12552:
URL: https://github.com/apache/lucene/pull/12552#issuecomment-1726315384
Thanks for the CHANGES entry - I'll push shortly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] Tony-X commented on pull request #12552: Make FSTPostingsFormat load FSTs off-heap
Posted by "Tony-X (via GitHub)" <gi...@apache.org>.
Tony-X commented on PR #12552:
URL: https://github.com/apache/lucene/pull/12552#issuecomment-1719878383
@mikemccand hey Mike, I did not make a new Codec for this. IIRC, `FSTPostingsFormat` will be exercised by the RandomCodec. Also there is `TestFSTPostingsFormat extends BasePostingsFormatTestCase` that tested this PostingsFormat.
Oh wait, I see that the test target support `-Dtests.postingsformat` to override posting format. So I ran `./gradlew test -Dtests.postingsformat=FST50
` and all tests passed.
Since the successful output doesn't provide randomization info, I also ran with an non-existent postings format `./gradlew test -Dtests.postingsformat=FST5x` and it failed, which means the override is happening.
`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org