You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "dungba88 (via GitHub)" <gi...@apache.org> on 2023/12/26 09:15:32 UTC
[PR] lazily write the FST padding byte [lucene]
dungba88 opened a new pull request, #12981:
URL: https://github.com/apache/lucene/pull/12981
### Description
Lazily write the FST padding byte, so that in case the FST is empty (no accepted nodes) nothing will be written. This is important for off-heap writing, as we don't want to add that extra byte when the FST would be thrown away. Found while working on https://github.com/apache/lucene/pull/12980
--
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] Lazily write the FST padding byte [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand merged PR #12981:
URL: https://github.com/apache/lucene/pull/12981
--
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] Lazily write the FST padding byte [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12981:
URL: https://github.com/apache/lucene/pull/12981#issuecomment-1884850269
Egads, thank you bot! This one had already fallen past the event horizon of my email box. I'll try to review soon.
--
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] Lazily write the FST padding byte [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #12981:
URL: https://github.com/apache/lucene/pull/12981#discussion_r1447386945
##########
lucene/core/src/java/org/apache/lucene/util/fst/FSTCompiler.java:
##########
@@ -164,14 +164,12 @@ private FSTCompiler(
boolean allowFixedLengthArcs,
DataOutput dataOutput,
float directAddressingMaxOversizingFactor,
- int version)
- throws IOException {
+ int version) {
this.allowFixedLengthArcs = allowFixedLengthArcs;
this.directAddressingMaxOversizingFactor = directAddressingMaxOversizingFactor;
this.version = version;
// pad: ensure no node gets address 0 which is reserved to mean
- // the stop state w/ no arcs
- dataOutput.writeByte((byte) 0);
+ // the stop state w/ no arcs. the actual byte will be written lazily
Review Comment:
Hmm could we instead add a boolean `paddingBytePending` or so? Set it to true, here, then when the byte is lazily written, set it to false, write the byte, and increment `numBytesWritten` at that point? Otherwise it's sort of weird to increment `numBytesWritten` when we didn't actually write the byte yet?
--
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] Lazily write the FST padding byte [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12981:
URL: https://github.com/apache/lucene/pull/12981#issuecomment-1883992139
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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] Lazily write the FST padding byte [lucene]
Posted by "dungba88 (via GitHub)" <gi...@apache.org>.
dungba88 commented on code in PR #12981:
URL: https://github.com/apache/lucene/pull/12981#discussion_r1447430604
##########
lucene/core/src/java/org/apache/lucene/util/fst/FSTCompiler.java:
##########
@@ -164,14 +164,12 @@ private FSTCompiler(
boolean allowFixedLengthArcs,
DataOutput dataOutput,
float directAddressingMaxOversizingFactor,
- int version)
- throws IOException {
+ int version) {
this.allowFixedLengthArcs = allowFixedLengthArcs;
this.directAddressingMaxOversizingFactor = directAddressingMaxOversizingFactor;
this.version = version;
// pad: ensure no node gets address 0 which is reserved to mean
- // the stop state w/ no arcs
- dataOutput.writeByte((byte) 0);
+ // the stop state w/ no arcs. the actual byte will be written lazily
Review Comment:
numBytesWritten is used for determine the address of the to be written nodes, so if we don't increment it here, it would mess up the address.
--
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] Lazily write the FST padding byte [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12981:
URL: https://github.com/apache/lucene/pull/12981#issuecomment-1885233436
> > How serious is this, during off-heap writing? Are we using off-heap writing anywhere in Lucene yet (e.g. block tree)?
>
> This will break the off-heap writing, as we are writting byte which won't belong to any FST, and it would mess up subsequent reads (if multiple FST are written to the same file)
Oh no, I see. Hmm, well could we increment the `numBytesWritten` but use a separately boolean indicating that the lazy byte hasn't been written? It's spooky to have `numBytesWritten = 1` mean it may or may not have been written.
--
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] Lazily write the FST padding byte [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #12981:
URL: https://github.com/apache/lucene/pull/12981#discussion_r1447388445
##########
lucene/core/src/java/org/apache/lucene/util/fst/FSTCompiler.java:
##########
@@ -552,13 +550,22 @@ long addNode(FSTCompiler.UnCompiledNode<T> nodeIn) throws IOException {
}
reverseScratchBytes();
+ // write the padding byte if needed
+ if (numBytesWritten == 1) {
+ writePaddingByte();
+ }
scratchBytes.writeTo(dataOutput);
numBytesWritten += scratchBytes.getPosition();
nodeCount++;
return numBytesWritten - 1;
}
+ private void writePaddingByte() throws IOException {
+ assert numBytesWritten == 1;
Review Comment:
Maybe add a comment explaining what this padding byte even is for? I myself cannot remember :)
--
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] Lazily write the FST padding byte [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #12981:
URL: https://github.com/apache/lucene/pull/12981#discussion_r1447387657
##########
lucene/core/src/java/org/apache/lucene/util/fst/FSTCompiler.java:
##########
@@ -968,6 +975,9 @@ public FST<T> compile() throws IOException {
if (root.numArcs == 0) {
if (fst.metadata.emptyOutput == null) {
return null;
Review Comment:
Maybe add a comment that this means a completely empty FST? Accepts nothing?
--
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] Lazily write the FST padding byte [lucene]
Posted by "dungba88 (via GitHub)" <gi...@apache.org>.
dungba88 commented on PR #12981:
URL: https://github.com/apache/lucene/pull/12981#issuecomment-1886114456
Thanks @mikemccand for reviewing, I've addressed those in the latest revision.
--
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] Lazily write the FST padding byte [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12981:
URL: https://github.com/apache/lucene/pull/12981#issuecomment-1885233927
> We are not using off-heap, but I have 2 other PR to start doing that:
OK, phew, thanks. Those will be next up :)
--
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] Lazily write the FST padding byte [lucene]
Posted by "dungba88 (via GitHub)" <gi...@apache.org>.
dungba88 commented on PR #12981:
URL: https://github.com/apache/lucene/pull/12981#issuecomment-1884917345
> How serious is this, during off-heap writing? Are we using off-heap writing anywhere in Lucene yet (e.g. block tree)?
This will break the off-heap writing, as we are writting byte which won't belong to any FST, and it would mess up subsequent reads (if multiple FST are written to the same file)
We are not using off-heap, but I have 2 other PR to start doing that:
- https://github.com/apache/lucene/pull/12980
- https://github.com/apache/lucene/pull/12985
--
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