Hi Peter,
> On 25 Nov 2015, at 12:21, Peter Levart <peter.lev...@gmail.com> wrote:
>
> 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 }
>
Doh, thanks i will update to use those constants.
>
> 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?).
Right “yet", we were contemplating how useful it would be for ref types, with
the likely hood being it would bail out of the mismatch more frequently (far
more so than say that for NaNs of float/double). At this point there seems to
be little value using it for ref arrays.
> 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.
We are in Unsafe territory :-)
Since the Java mismatch method and the intrinsic only make temporary reads,
IIUC such updates by GC to the array can only occur at a safe points, and i
don’t think int loops produce a safe point check within the hot loop and nor
does the intrinsic, is this really an issue?
I forgot to mention in my TODO list that i have plans to reuse the unsafe
vectorizedMismatch method within nio heap/direct buffers, so it is likely
ArraysSupport will get moved to some jdk.internal.* package at some point.
Paul.
>
> 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.deshpa...@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
>
