RFR: 8255150: Add utility methods to check long indexes and ranges

[Jorn Vernee]

On Mon, 2 Nov 2020 10:47:18 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> This change add 3 new methods in Objects:
> 
> public static long checkIndex(long index, long length)
> public static long checkFromToIndex(long fromIndex, long toIndex, long length)
> public static long checkFromIndexSize(long fromIndex, long size, long length)
> 
> This mirrors the int utility methods that were added by JDK-8135248
> with the same motivations.
> 
> As is the case with the int checkIndex(), the long checkIndex() method
> is JIT compiled as an intrinsic. It allows the JIT to compile
> checkIndex to an unsigned comparison and properly recognize it as
> a range check that then becomes a candidate for the existing range check
> optimizations. This has proven to be important for panama's
> MemorySegment API and a prototype of this change (with some extra c2
> improvements) showed that panama micro benchmark results improve
> significantly.
> 
> This change includes:
> 
> - the API change
> - the C2 intrinsic
> - tests for the API and the C2 intrinsic
> 
> This is a joint work with Paul who reviewed and reworked the API change
> and filled the CSR.

Hi Roland, Paul. Thank you for tackling this! We have to jump through quite a few hoops in the implementation of the foreign memory access API in order to leverage the intrinsification of int-based index checks, and even then we are not covering the cases where the numbers are larger than ints. Looking forward to being able to remove those hacks!

Although I'm not a 'Reviewer', I've done a pass over the code and left some inline comments that I hope you find useful.

src/hotspot/share/opto/graphKit.hpp line 109:

> 107:     if (bt == T_INT) {
> 108:       assert((jlong)(jint)con == con, "not an int");
> 109:       return intcon((jint) con);

Could also use `checked_cast` here (from globalDefinitions.hpp), which does a similar check.
Suggestion:

      return intcon(checked_cast<jint>(con));

src/hotspot/share/opto/graphKit.hpp line 111:

> 109:       return intcon((jint) con);
> 110:     }
> 111:     return longcon(con);

This ends up defaulting to longcon for any basic type that is not T_INT. Maybe it's nice to have an assert here?
Suggestion:

    assert(bt == T_LONG, "basic type not an int or long");
    return longcon(con);

src/hotspot/share/opto/type.cpp line 1353:

> 1351:     jint int_hi = (jint)hi;
> 1352:     assert(((jlong)int_lo) == lo && ((jlong)int_hi) == hi, "bounds are not ints");
> 1353:     return TypeInt::make(int_lo, int_hi, w);

checked_cast could also be used here.
Suggestion:

    return TypeInt::make(checked_cast<jint>(lo), checked_cast<jint>(hi), w);

src/java.base/share/classes/java/lang/IndexOutOfBoundsException.java line 83:

> 81:      */
> 82:     public IndexOutOfBoundsException(long index) {
> 83:         super("Index out of range: " + index);

For consistency with the class name:
Suggestion:

        super("Index out of bounds: " + index);

test/jdk/java/util/Objects/CheckLongIndex.java line 94:

> 92:         long apply(long a, long b, long c);
> 93:     }
> 94: 

Seems unused?
Suggestion:

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

PR: https://git.openjdk.java.net/jdk/pull/1003