Re: RFR: Here are some easy patches

[Martin Buchholz]

On Wed, May 2, 2018 at 2:43 PM, Michael Rasmussen <
michael.rasmus...@roguewave.com> wrote:

> But getComponentType itself calls isArray, so you are paying the native
> method overhead anyway (though it is intrinsic).
>

Ohhh, I had forgotten I had looked at  getComponentType earlier.  So
calling getArray directly should not really give any improvement for leaf
nodes.

I tried the variant below, but it gives no performance difference on my
naive non-jmh benchmark.  (you are benchmarking jdk11 head, right)

public static int deepHashCode(Object a[]) {
if (a == null)
return 0;

int result = 1;

for (Object element : a) {
final int elementHash;
final Class cl;
if (element == null)
elementHash = 0;
else if (!(cl = element.getClass()).isArray())
elementHash = element.hashCode();
else if (element instanceof Object[])
elementHash = deepHashCode((Object[]) element);
else
elementHash = primitiveArrayHashCode(element, cl);

result = 31 * result + elementHash;
}

return result;
}

private static int primitiveArrayHashCode(Object a, Class cl) {
return
(cl == byte[].class)? hashCode((byte[]) a):
(cl == int[].class) ? hashCode((int[]) a) :
(cl == long[].class)? hashCode((long[]) a):
(cl == char[].class)? hashCode((char[]) a):
(cl == short[].class)   ? hashCode((short[]) a)   :
(cl == boolean[].class) ? hashCode((boolean[]) a) :
(cl == double[].class)  ? hashCode((double[]) a)  :
// If new primitive types are ever added, this method must be
// expanded or we will fail here with ClassCastException.
hashCode((float[]) a);
}

Re: RFR: Here are some easy patches

[Michael Rasmussen]

But getComponentType itself calls isArray, so you are paying the native method 
overhead anyway (though it is intrinsic).


I haven't dug further into the JIT generated code to see why isArray performs 
better though.


/Michael


From: Martin Buchholz 
Sent: 03 May 2018 00:13
To: Michael Rasmussen
Cc: core-libs-dev
Subject: Re: RFR: Here are some easy patches

Michael,  Thanks.

This may be tricky.  isArray is a native method, and we don't want to pay for 
native method overhead - we're depending on hotspot intrinsification.  I 
suspect isArray will lose with -Xint and perhaps also with C1.  In the hotspot 
sources I see an ominous
virtual bool is_array_klass_slow().  Perhaps other engineers can give an 
authoritative recommendation on which way to go.

Re: RFR: Here are some easy patches

[Martin Buchholz]

Michael,  Thanks.

This may be tricky.  isArray is a native method, and we don't want to pay
for native method overhead - we're depending on hotspot intrinsification.
I suspect isArray will lose with -Xint and perhaps also with C1.  In the
hotspot sources I see an ominous
virtual bool is_array_klass_slow().  Perhaps other engineers can give an
authoritative recommendation on which way to go.


On Wed, May 2, 2018 at 1:51 PM, Michael Rasmussen <
michael.rasmus...@roguewave.com> wrote:

> Hi Martin,
>
>
> Did you consider using Class::isArray in the loop? Something like
> the following:
>
> for (Object element : a) {
>   final int elementHash;
>   if (element == null) {
> elementHash = 0;
>   }
>   else {
> final Class cl = element.getClass();
> if (!cl.isArray())
>   elementHash = element.hashCode();
> else if (element instanceof Object[])
>   elementHash = deepHashCode((Object[]) element);
> else
>   elementHash = primitiveArrayHashCode(element,
> cl.getComponentType());
>   }
>
>   result = 31 * result + elementHash;
> }
>
> In my quick JMH test running on Java 10, it improved the performance
> with your test array (Object[10] full of Integers) from 244 us/op to
> 160 us/op (vs current JDK: 399 us/op).
>
>
> /Michael
> --
> *From:* core-libs-dev  on behalf
> of Martin Buchholz 
> *Sent:* 02 May 2018 21:17:19
> *To:* Paul Sandoz
> *Cc:* core-libs-dev
> *Subject:* Re: RFR: Here are some easy patches
>
> Hi Paul,
>
> On Mon, Apr 30, 2018 at 2:03 PM, Paul Sandoz 
> wrote:
>
> >
> >
> > On Apr 30, 2018, at 11:18 AM, Martin Buchholz 
> wrote:
> >
> >
> >
> > On Mon, Apr 30, 2018 at 10:35 AM, Paul Sandoz 
> > wrote:
> >
> >> An obvious optimization:
> >>
> >> 8202398: Optimize Arrays.deepHashCode
> >> http://cr.openjdk.java.net/~martin/webrevs/jdk/deepHashCode-optimize/
> >> https://bugs.openjdk.java.net/browse/JDK-8202398
> >>
> >> I would prefer that the deeply nested ternary expressions be changed to
> a
> >> more expected if/else if/else
> >>
> >
> > My brain much prefers code transforming tabular data to "look tabular".
> >
> >
> >
> > I think you will like expression switch :-) in the interim i would stick
> > with the less eyebrow raising syntax.
> >
> >
> I'm going to claim committer's privilege and check in with my preferred
> tabular style.  You can rewrite using suburban sprawl style when us
> dinosaurs from the last milllenium have gone extinct.
>

Re: RFR: Here are some easy patches

[Michael Rasmussen]

Hi Martin,


Did you consider using Class::isArray in the loop? Something like the following:

for (Object element : a) {
  final int elementHash;
  if (element == null) {
elementHash = 0;
  }
  else {
final Class cl = element.getClass();
if (!cl.isArray())
  elementHash = element.hashCode();
else if (element instanceof Object[])
  elementHash = deepHashCode((Object[]) element);
else
  elementHash = primitiveArrayHashCode(element, cl.getComponentType());
  }

  result = 31 * result + elementHash;
}


In my quick JMH test running on Java 10, it improved the performance with your 
test array (Object[10] full of Integers) from 244 us/op to 160 us/op (vs 
current JDK: 399 us/op).


/Michael


From: core-libs-dev  on behalf of 
Martin Buchholz 
Sent: 02 May 2018 21:17:19
To: Paul Sandoz
Cc: core-libs-dev
Subject: Re: RFR: Here are some easy patches

Hi Paul,

On Mon, Apr 30, 2018 at 2:03 PM, Paul Sandoz  wrote:

>
>
> On Apr 30, 2018, at 11:18 AM, Martin Buchholz  wrote:
>
>
>
> On Mon, Apr 30, 2018 at 10:35 AM, Paul Sandoz 
> wrote:
>
>> An obvious optimization:
>>
>> 8202398: Optimize Arrays.deepHashCode
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/deepHashCode-optimize/
>> https://bugs.openjdk.java.net/browse/JDK-8202398
>>
>> I would prefer that the deeply nested ternary expressions be changed to a
>> more expected if/else if/else
>>
>
> My brain much prefers code transforming tabular data to "look tabular".
>
>
>
> I think you will like expression switch :-) in the interim i would stick
> with the less eyebrow raising syntax.
>
>
I'm going to claim committer's privilege and check in with my preferred
tabular style.  You can rewrite using suburban sprawl style when us
dinosaurs from the last milllenium have gone extinct.

Re: RFR: Here are some easy patches

[Martin Buchholz]

Hi Paul,

On Mon, Apr 30, 2018 at 2:03 PM, Paul Sandoz  wrote:

>
>
> On Apr 30, 2018, at 11:18 AM, Martin Buchholz  wrote:
>
>
>
> On Mon, Apr 30, 2018 at 10:35 AM, Paul Sandoz 
> wrote:
>
>> An obvious optimization:
>>
>> 8202398: Optimize Arrays.deepHashCode
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/deepHashCode-optimize/
>> https://bugs.openjdk.java.net/browse/JDK-8202398
>>
>> I would prefer that the deeply nested ternary expressions be changed to a
>> more expected if/else if/else
>>
>
> My brain much prefers code transforming tabular data to "look tabular".
>
>
>
> I think you will like expression switch :-) in the interim i would stick
> with the less eyebrow raising syntax.
>
>
I'm going to claim committer's privilege and check in with my preferred
tabular style.  You can rewrite using suburban sprawl style when us
dinosaurs from the last milllenium have gone extinct.

Re: RFR: Here are some easy patches

[Paul Sandoz]

> On Apr 30, 2018, at 11:18 AM, Martin Buchholz  wrote:
> 
> 
> 
> On Mon, Apr 30, 2018 at 10:35 AM, Paul Sandoz  > wrote:
>> An obvious optimization:
>> 
>> 8202398: Optimize Arrays.deepHashCode
>> http://cr.openjdk.java.net/~martin/webrevs/jdk/deepHashCode-optimize/ 
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8202398 
>> 
>> 
> 
> I would prefer that the deeply nested ternary expressions be changed to a 
> more expected if/else if/else
> 
> My brain much prefers code transforming tabular data to "look tabular".
>  

I think you will like expression switch :-) in the interim i would stick with 
the less eyebrow raising syntax.


> , and the last else throws an InternalError(“Should not reach here”) with 
> some such message if you really want to express the case of an undetected 
> array type.
> 
> I'd hope valhalla engineers will catch it before anyone else.  In fact, 
> perhaps it's not too early to add a test as part of the valhalla project now? 

I believe in principle it should just work (assuming hash code support for 
values themselves) because an array of values is currently a subtype of 
Object[].

At some point we do need to investigate the L-world with tests for values and 
arrays/collections of, that will help test functionality but also expose issues 
around nulls. The 166 TCK might be a good place to start.

Paul.

Re: RFR: Here are some easy patches

[Martin Buchholz]

On Mon, Apr 30, 2018 at 10:35 AM, Paul Sandoz 
wrote:

> An obvious optimization:
>
> 8202398: Optimize Arrays.deepHashCode
> http://cr.openjdk.java.net/~martin/webrevs/jdk/deepHashCode-optimize/
> https://bugs.openjdk.java.net/browse/JDK-8202398
>
> I would prefer that the deeply nested ternary expressions be changed to a
> more expected if/else if/else
>

My brain much prefers code transforming tabular data to "look tabular".


> , and the last else throws an InternalError(“Should not reach here”) with
> some such message if you really want to express the case of an undetected
> array type.
>

I'd hope valhalla engineers will catch it before anyone else.  In fact,
perhaps it's not too early to add a test as part of the valhalla project
now?

Re: RFR: Here are some easy patches

[Paul Sandoz]

> On Apr 30, 2018, at 9:05 AM, Martin Buchholz  wrote:
> 
> Off by one pixel!
> 
> 8202397: Typo in X-Buffer javadoc
> http://cr.openjdk.java.net/~martin/webrevs/jdk/X-Buffer-typo/ 
> 
> https://bugs.openjdk.java.net/browse/JDK-8202397 
> 
> 

+1


> Fixes a long-term micro-embarrassment:
> 
> 8201634: Random seedUniquifier uses incorrect LCG
> http://cr.openjdk.java.net/~martin/webrevs/jdk/Random-Ecuyer/ 
> 
> https://bugs.openjdk.java.net/browse/JDK-8201634 
> 
> 

Finally! that should put to rest any numerology on the matter :-)


> An obvious optimization:
> 
> 8202398: Optimize Arrays.deepHashCode
> http://cr.openjdk.java.net/~martin/webrevs/jdk/deepHashCode-optimize/ 
> 
> https://bugs.openjdk.java.net/browse/JDK-8202398 
> 
> 


I would prefer that the deeply nested ternary expressions be changed to a more 
expected if/else if/else, and the last else throws an InternalError(“Should not 
reach here”) with some such message if you really want to express the case of 
an undetected array type.

—

For performance i suppose we could, when value types arrive, special case 
arrays of values, where the component value type has no references (direct or 
indirect) if that is even possible to efficiently detect i.e. a value type that 
is like a primitive. (Note, at the moment in the Valhalla L-world proposal 
arrays of values are subtypes of Object[]). 

Paul.