You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/11/11 21:02:57 UTC
[GitHub] [lucene] rmuir opened a new pull request, #11923: enable error-prone "narrow calculation" check
rmuir opened a new pull request, #11923:
URL: https://github.com/apache/lucene/pull/11923
This check finds bugs such as https://github.com/apache/lucene/pull/11916.
For that particular bug, the error messages look like this:
```
/home/rmuir/workspace/lucene/lucene/core/src/java/org/apache/lucene/codecs/lucene94/Lucene94HnswVectorsReader.java:402: warning: [NarrowCalculation] This product of integers could overflow before being implicitly
cast to a long.
graphOffsetsByLevel[level] = (1 + (M * 2)) * Integer.BYTES * numNodesOnLevel0;
^
(see https://errorprone.info/bugpattern/NarrowCalculation)
Did you mean 'graphOffsetsByLevel[level] = (1 + (M * 2)) * Integer.BYTES * ((long) numNodesOnLevel0);'?
/home/rmuir/workspace/lucene/lucene/core/src/java/org/apache/lucene/codecs/lucene94/Lucene94HnswVectorsReader.java:406: warning: [NarrowCalculation] This product of integers could overflow before being implicitly
cast to a long.
graphOffsetsByLevel[level - 1] + (1 + M) * Integer.BYTES * numNodesOnPrevLevel;
^
(see https://errorprone.info/bugpattern/NarrowCalculation)
Did you mean 'graphOffsetsByLevel[level - 1] + (1 + M) * Integer.BYTES * ((long) numNodesOnPrevLevel);'?
/home/rmuir/workspace/lucene/lucene/core/src/java/org/apache/lucene/codecs/lucene94/Lucene94HnswVectorsReader.java:440: warning: [NarrowCalculation] This product of integers could overflow before being implicitly cast to a long.
this.bytesForConns0 = ((long) (entry.M * 2) + 1) * Integer.BYTES;
^
(see https://errorprone.info/bugpattern/NarrowCalculation)
Did you mean 'this.bytesForConns0 = ((long) (entry.M * 2L) + 1) * Integer.BYTES;'?
```
See https://errorprone.info/bugpattern/NarrowCalculation for more information.
Closes #11910
--
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] rmuir commented on pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
rmuir commented on PR #11923:
URL: https://github.com/apache/lucene/pull/11923#issuecomment-1312221930
This NarrowCalculation is specific to multiplication, which was the issue for #11905. It would not have detected the multiplication issue for the bug before that (#11861), as that one never involved `long` at all, just a pure integer overflow in integer space.
Instead here, the idea is that its always suspicious if you see `long z = x * y`, where x and y are integers. A good example is simpletext code doing stuff like `seek(docid * 8)` which is bad, it will overflow at 270M docs. so you should do `seek(docid * 8L)` instead.
The "IntLongMath" is more aggressive and looks at not just multiplication but other operations too. example of other stuff it picks up:
```
lucene/core/src/java/org/apache/lucene/search/DocIdSetIterator.java:131: warning: [IntLongMath] Expression of type int may overflow before being assigned to a long
return maxDoc - minDoc;
^
(see https://errorprone.info/bugpattern/IntLongMath)
Did you mean 'return (long) maxDoc - minDoc;'?
```
Hence I'm not sure about enabling `IntLongMath`, I think it might be more of a hassle than it is worth. I feel like this `NarrowCalculation` is a decent tradeoff. Multiplication was responsible for the last 2 overflow bugs in vectors. Normally when multiplying the developer starts thinks about overflow already, so the suggestions that it makes are not unnatural.
--
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] uschindler commented on a diff in pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
uschindler commented on code in PR #11923:
URL: https://github.com/apache/lucene/pull/11923#discussion_r1020891869
##########
lucene/core/src/java/org/apache/lucene/util/BytesRefHash.java:
##########
@@ -414,7 +414,7 @@ private void rehash(final int newSize, boolean hashOnData) {
}
hashMask = newMask;
- bytesUsed.addAndGet(Integer.BYTES * (-ids.length));
Review Comment:
This is always a bit hard to read. Mabye we should add a method `Counter#subtractAndGet` in our counter class? I think this might be a separate issue, but might worth to do it.
An alternative to this line: In most cases in this issue you applied the cast to "constants" (because then the javac compiler can do the inlining directly (you know, javac inlines static final integer and String literals). So maybe have it here like `(long) Integer.BYTES * (-ids.length)`.
Sorry for bringing this specific place up again.
--
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] rmuir commented on a diff in pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
rmuir commented on code in PR #11923:
URL: https://github.com/apache/lucene/pull/11923#discussion_r1022818839
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterFlushControl.java:
##########
@@ -85,7 +85,7 @@ final class DocumentsWriterFlushControl implements Accountable, Closeable {
this.perThreadPool = documentsWriter.perThreadPool;
this.flushPolicy = config.getFlushPolicy();
this.config = config;
- this.hardMaxBytesPerDWPT = config.getRAMPerThreadHardLimitMB() * 1024 * 1024;
+ this.hardMaxBytesPerDWPT = config.getRAMPerThreadHardLimitMB() * 1024L * 1024L;
Review Comment:
sounds good to me! if you want to make a PR with more cleanups/safety/fixes, let's get them in.
--
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] uschindler commented on pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #11923:
URL: https://github.com/apache/lucene/pull/11923#issuecomment-1312717445
@rmuir just some question about cases like `merge.estimatedMergeBytes / 1024 / 1024.` changed to `merge.estimatedMergeBytes / 1024. / 1024.`:
Were they also found by this check or did you just change them because you have ssen them. Because the pattern here is different. Division with integers should not overflow, so I wonder why this was found by the errorprone check.
The new code is much better as it is consistent!
--
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] rmuir commented on a diff in pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
rmuir commented on code in PR #11923:
URL: https://github.com/apache/lucene/pull/11923#discussion_r1020892411
##########
lucene/core/src/java/org/apache/lucene/util/BytesRefHash.java:
##########
@@ -414,7 +414,7 @@ private void rehash(final int newSize, boolean hashOnData) {
}
hashMask = newMask;
- bytesUsed.addAndGet(Integer.BYTES * (-ids.length));
Review Comment:
i reverted the last commit.
--
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] rmuir commented on a diff in pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
rmuir commented on code in PR #11923:
URL: https://github.com/apache/lucene/pull/11923#discussion_r1020582937
##########
lucene/core/src/java/org/apache/lucene/util/BytesRefHash.java:
##########
@@ -414,7 +414,7 @@ private void rehash(final int newSize, boolean hashOnData) {
}
hashMask = newMask;
- bytesUsed.addAndGet(Integer.BYTES * (-ids.length));
+ bytesUsed.addAndGet(Integer.BYTES * (long) -ids.length);
Review Comment:
yes we can improve these. i did a lot of the RAM usage stuff kinda mechanically, there was a fair amount of 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] rmuir merged pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
rmuir merged PR #11923:
URL: https://github.com/apache/lucene/pull/11923
--
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] rmuir commented on pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
rmuir commented on PR #11923:
URL: https://github.com/apache/lucene/pull/11923#issuecomment-1314486480
> just some question about cases like `merge.estimatedMergeBytes / 1024 / 1024.` changed to `merge.estimatedMergeBytes / 1024. / 1024.`:
> Were they also found by this check or did you just change them because you have ssen them. Because the pattern here is different. Division with integers should not overflow, so I wonder why this was found by the errorprone check.
> The new code is much better as it is consistent!
yes, some of the time the code was doing `X / 1024 / 1024.` and other times it was doing `X / 1024. / 1024.` So it is the floating point portion of this NarrowCalculation check. It doesn't overflow when you do this, but it unnecessarily throws some precision away in the trash... so it is fixed to be consistent everywhere. I think for the cases we have it would not cause any bad bugs, but it was also easy enough to fix so that we can have the check enabled everywhere.
--
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] risdenk commented on a diff in pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #11923:
URL: https://github.com/apache/lucene/pull/11923#discussion_r1020579969
##########
lucene/core/src/java/org/apache/lucene/util/BytesRefHash.java:
##########
@@ -414,7 +414,7 @@ private void rehash(final int newSize, boolean hashOnData) {
}
hashMask = newMask;
- bytesUsed.addAndGet(Integer.BYTES * (-ids.length));
+ bytesUsed.addAndGet(Integer.BYTES * (long) -ids.length);
Review Comment:
Would it be clearer as:
```
bytesUsed.addAndGet(Integer.BYTES * -1L * ids.length);
```
--
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] uschindler commented on a diff in pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
uschindler commented on code in PR #11923:
URL: https://github.com/apache/lucene/pull/11923#discussion_r1020582330
##########
lucene/core/src/java/org/apache/lucene/util/BytesRefHash.java:
##########
@@ -414,7 +414,7 @@ private void rehash(final int newSize, boolean hashOnData) {
}
hashMask = newMask;
- bytesUsed.addAndGet(Integer.BYTES * (-ids.length));
+ bytesUsed.addAndGet(Integer.BYTES * (long) -ids.length);
Review Comment:
Yeah that's better!
--
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] uschindler commented on pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #11923:
URL: https://github.com/apache/lucene/pull/11923#issuecomment-1312718213
> @rmuir just some question about cases like `merge.estimatedMergeBytes / 1024 / 1024.` changed to `merge.estimatedMergeBytes / 1024. / 1024.`: Were they also found by this check or did you just change them because you have ssen them. Because the pattern here is different. Division with integers should not overflow, so I wonder why this was found by the errorprone check. The new code is much better as it is consistent!
A sorry, yes it is a bug for small integers. It is explained in the errorprone bug docs: https://errorprone.info/bugpattern/NarrowCalculation
Sorry for asking!
--
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] risdenk commented on a diff in pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #11923:
URL: https://github.com/apache/lucene/pull/11923#discussion_r1020580110
##########
lucene/core/src/java/org/apache/lucene/util/BytesRefHash.java:
##########
@@ -414,7 +414,7 @@ private void rehash(final int newSize, boolean hashOnData) {
}
hashMask = newMask;
- bytesUsed.addAndGet(Integer.BYTES * (-ids.length));
+ bytesUsed.addAndGet(Integer.BYTES * (long) -ids.length);
Review Comment:
Same goes for a bunch of the negative ones above.
--
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] rmuir commented on pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
rmuir commented on PR #11923:
URL: https://github.com/apache/lucene/pull/11923#issuecomment-1312641592
@uschindler @risdenk if you want to take another pass, it would be helpful. I don't want to introduce new risks here with this change.
--
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] rmuir commented on pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
rmuir commented on PR #11923:
URL: https://github.com/apache/lucene/pull/11923#issuecomment-1312717162
just hash out how you want the style of the bytesrefhash code to be as we are going back and forth here.
a little sad as i thought spotless was the end of the java style bikesheds :)
ill check back in a few days.
--
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] uschindler commented on pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
uschindler commented on PR #11923:
URL: https://github.com/apache/lucene/pull/11923#issuecomment-1312717771
All fine! I vote: merge this ASAP :-)
--
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] benwtrent commented on a diff in pull request #11923: enable error-prone "narrow calculation" check
Posted by GitBox <gi...@apache.org>.
benwtrent commented on code in PR #11923:
URL: https://github.com/apache/lucene/pull/11923#discussion_r1022817060
##########
lucene/core/src/java/org/apache/lucene/index/DocumentsWriterFlushControl.java:
##########
@@ -85,7 +85,7 @@ final class DocumentsWriterFlushControl implements Accountable, Closeable {
this.perThreadPool = documentsWriter.perThreadPool;
this.flushPolicy = config.getFlushPolicy();
this.config = config;
- this.hardMaxBytesPerDWPT = config.getRAMPerThreadHardLimitMB() * 1024 * 1024;
+ this.hardMaxBytesPerDWPT = config.getRAMPerThreadHardLimitMB() * 1024L * 1024L;
Review Comment:
i *think* the thing to do here would have been
`Math.multiplyExact(config.getRAMPerThreadHardLimitMB(), 1024 * 1024);` It seems it's invalid if this is every more than can be contained in an integer?:
https://github.com/apache/lucene/blob/b5795db0cf517f8942eed868752249df9b105603/lucene/core/src/java/org/apache/lucene/index/IndexWriterConfig.java#L343-L359
--
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