enable error-prone "narrow calculation" check

[rmuir]

This check finds bugs such as #11905.

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

[uschindler]

Nice changes and it found good overflow bugs!

[rmuir]

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.

[rmuir]

@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.

[uschindler]

I approve this. See also my comment about the idea to extend Counter class by subtractAndGet

[uschindler]

I aprove the original version again. Thanks!

[rmuir]

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.

[uschindler]

@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!

[uschindler]

All fine! I vote: merge this ASAP :-)

[uschindler]

@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!

[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!

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.

[benwtrent]

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?:

lucene/lucene/core/src/java/org/apache/lucene/index/IndexWriterConfig.java

Lines 343 to 359 in b5795db

	/**
	* Expert: Sets the maximum memory consumption per thread triggering a forced flush if exceeded. A
	* {@link DocumentsWriterPerThread} is forcefully flushed once it exceeds this limit even if the
	* {@link #getRAMBufferSizeMB()} has not been exceeded. This is a safety limit to prevent a {@link
	* DocumentsWriterPerThread} from address space exhaustion due to its internal 32 bit signed
	* integer based memory addressing. The given value must be less that 2GB (2048MB)
	*
	* @see #DEFAULT_RAM_PER_THREAD_HARD_LIMIT_MB
	*/
	public IndexWriterConfig setRAMPerThreadHardLimitMB(int perThreadHardLimitMB) {
	if (perThreadHardLimitMB <= 0 || perThreadHardLimitMB >= 2048) {
	throw new IllegalArgumentException(
	"PerThreadHardLimit must be greater than 0 and less than 2048MB");
	}
	this.perThreadHardLimitMB = perThreadHardLimitMB;
	return this;
	}

[rmuir]

sounds good to me! if you want to make a PR with more cleanups/safety/fixes, let's get them in.