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