[foreign-preview] RFR: 8282070: Drop workaround from memory segment implementation

[Maurizio Cimadamore]

On Thu, 17 Feb 2022 21:26:02 GMT, Radoslaw Smogura <duke at openjdk.java.net> wrote:

>> src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 392:
>> 
>>> 390:     @ForceInline
>>> 391:     void checkBounds(long offset, long length) {
>>> 392:         if (length > 0) {
>> 
>> This is the best I could do, given that `Objects.checkIndex` implements a <= semantics on the upper bound. That is, we need to protect the code from overflow, which can happen if `length == 0` (which can happen in slicing). When that happens, the upper bound in the second check `this.length - length + 1` ends up being `this.length + 1`, which is problematic. I've also tested to see if we could just use the old check bounds routine, but it seems that using `Objects.checkIndex` is required to properly apply BCE (I could see differences in the benchmark results).
>
> I think `Objects.checkIndex` checks if `length` && `offset` are negative, so there's no risk of overflow (actually it checks if `length` is negative, and then make unsigned compare), so maybe I miss here something.
> 
> Just minor, I wonder if it will be better to use `Preconditions.checkIndex` - one method less to inline.

Consider a case of a segment whose length is Long.MAX_VALUE. Then you want to take a slice of size 0. This will throw an exception, as Long.MAX_VALUE + 1 will flip into negative territory.

Another problem is when the segment has size zero, in which case the check:


long checkedOffset = Objects.checkIndex(offset, this.length);


Will also fail. To make that pass we'd need to tweak that check to use `this.length + 1` again, and risk overflow. So I figured it was better just just special case when the accessed length was zero (which should happen rarely). Another option would be to have different checks for slicing and for access (since for access length can never be zero) - but then it duplicates the implementation, and there are also methods like `copy` which are left a bit in the middle.

As for inlining, I doubt that it would make a difference to call Preconditions directly, since it's all `@ForceInline`d.

-------------

PR: https://git.openjdk.java.net/panama-foreign/pull/644