RFR 8136924 Vectorized support for array equals/compare/mismatch using Unsafe

[Peter Levart]

Hi Paul,

Browsing through ArraysSupport. So far I noticed just two nits ...

You define those nice constants like LOG2_ARRAY_CHAR_INDEX_SCALE, etc., 
but then use hard-coded literals in mismatch methods:

  295     static int mismatch(char[] a, int aFromIndex,
  296                         char[] b, int bFromIndex,
  297                         int length) {
  298         int i = 0;
  299         if (length > 3) {
  300             int aOffset = Unsafe.ARRAY_CHAR_BASE_OFFSET + 
(aFromIndex << 1); // << here
  301             int bOffset = Unsafe.ARRAY_CHAR_BASE_OFFSET + 
(bFromIndex << 1); // << here
  302             i = vectorizedMismatch(
  303                     a, aOffset,
  304                     b, bOffset,
  305                     length, LOG2_ARRAY_CHAR_INDEX_SCALE);
  306             if (i >= 0)
  307                 return i;
  308             i = length - ~i;
  309         }
  310         for (; i < length; i++) {
  311             if (a[aFromIndex + i] != b[bFromIndex + i])
  312                 return i;must
  313         }
  314         return -1;
  315     }


The mentioning of "reference component types" in javadoc for 
vectorizedMismatch:

   52     /**
   53      * Find the relative index of the first mismatching pair of 
elements in two
   54      * arrays of the same component type.   For reference 
component types the
   55      * reference values (as opposed to what they reference) will 
be matched.
   56      * Pairs of elements will be tested in order relative to given 
offsets into
   57      * both arrays.

... is probably a left-over, since there is no caller of the method 
using reference arrays (yet?). You should probably mention that doing so 
is not safe. As you might know, object pointers are a "movable target". 
GC can rewrite them as it moves objects around and if you "cast" an 
object pointer (or two of them) into a long value and store it in a long 
variable, GC can't track that and update the value, so you might be 
comparing an old address of an object with new address of the same 
object and get a mismatch.

I don't know much about intrinsified code. Whether it can be interrupted 
by GC or not, so it might be able to compare object references directly, 
but then the bytecode version of the method would have to have a 
special-case for reference arrays if it is executed in this form anytime.


Regards, Peter

On 11/25/2015 10:53 AM, Paul Sandoz wrote:
> Hi,
>
> And this is the review for the Java part:
>
>    http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8136924-arrays-mismatch-vectorized-unsafe/webrev/
>
> Which will be updated to add @HotSpotIntrinsicCandidate when JDK-8143355 is pushed. [1]
>
> The plan is all reviewed changes will be pushed to hs-comp and then we follow up:
>
>    1) adding the intrinsic to other platforms
>
>    2) improving C1 (perhaps even the interpreter?) since the intrinsic is a stub which IIUC makes it easier to plug in.
>
>    3) take a swing at consolidating other equal/compare intrinsics, such as those for char[]/String-based equal/compare
>
>    4) adding methods to String such as mismatch method.
>
> I can help by pushing all reviewed patches. I will kick off a JPRT run with all patches applied.
>
> I did evaluate/test the HotSpot patch (stared at the patch and generated code for UseAVX < 2, and measured) and reviewed with my limited knowledge of HotSpot.
>
> Paul.
>
> [1]
> diff -r 01b49c2960fd src/java.base/share/classes/java/util/ArraysSupport.java
> --- a/src/java.base/share/classes/java/util/ArraysSupport.java	Tue Nov 17 15:42:53 2015 +0100
> +++ b/src/java.base/share/classes/java/util/ArraysSupport.java	Tue Nov 17 17:05:09 2015 +0100
> @@ -24,7 +24,7 @@
>   */
> package java.util;
>
> -//import jdk.internal.HotSpotIntrinsicCandidate;
> +import jdk.internal.HotSpotIntrinsicCandidate;
> import jdk.internal.misc.Unsafe;
>
> class ArraysSupport {
> @@ -72,7 +72,7 @@
>       * If a mismatch is not found the negation of one plus the number of
>       * remaining pairs of elements to be checked in the tail of the two arrays.
>       */
> -//    @HotSpotIntrinsicCandidate
> +    @HotSpotIntrinsicCandidate
>      static int vectorizedMismatch(Object a, long aOffset,
>                                    Object b, long bOffset,
>                                    int length,
>
>> On 25 Nov 2015, at 01:00, Deshpande, Vivek R <vivek.r.deshpande at intel.com> wrote:
>>
>> Hi all
>>
>> We would like to contribute a patch from Intel which optimizes vectorizedMismatch() method in java.util.ArraysSupport.java for X86 architecture using AVX instructions.
>> The improvement gives more than 2x gain over Unsafe implementation for long arrays.
>> The bug is blocked by bug: vectorized support for array equals/compare/mismatch using Unsafe (https://bugs.openjdk.java.net/browse/JDK-8136924.)
>> Could you please review and sponsor this patch.
>>
>> Bug-id:
>> https://bugs.openjdk.java.net/browse/JDK-8143355
>> webrev:
>> http://cr.openjdk.java.net/~mcberg/8143355/webrev.01/
>>
>> Thanks and regards,
>> Vivek