RFR: 8184777: Factor out species generation logic from BoundMethodHandle

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8184777: Factor out species generation logic from BoundMethodHandle

Claes Redestad
Hi,

  this patch factors out the BoundMethodHandle species data class
generation to a new ClassSpecializer facility.

  While currently semantically neutral, this will make it possible
to reuse the facility in other places.

  Webrev: http://cr.openjdk.java.net/~redestad/8184777/open.00/
  Bug: https://bugs.openjdk.java.net/browse/JDK-8184777

  Performance wise this adds a very small (~20k bytecode) amount
of work to the initialization costs of BMHs, which we expect will
be more than repaid as we apply the ClassSpecializer elsewhere.

  Thanks!

/Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184777: Factor out species generation logic from BoundMethodHandle

Vladimir Ivanov
Looks good!

Best regards,
Vladimir Ivanov

On 11/13/17 7:34 PM, Claes Redestad wrote:

> Hi,
>
>   this patch factors out the BoundMethodHandle species data class
> generation to a new ClassSpecializer facility.
>
>   While currently semantically neutral, this will make it possible
> to reuse the facility in other places.
>
>   Webrev: http://cr.openjdk.java.net/~redestad/8184777/open.00/
>   Bug: https://bugs.openjdk.java.net/browse/JDK-8184777
>
>   Performance wise this adds a very small (~20k bytecode) amount
> of work to the initialization costs of BMHs, which we expect will
> be more than repaid as we apply the ClassSpecializer elsewhere.
>
>   Thanks!
>
> /Claes
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8184777: Factor out species generation logic from BoundMethodHandle

Claes Redestad
Thanks Vladimir!

/Claes

On 2017-11-13 17:38, Vladimir Ivanov wrote:

> Looks good!
>
> Best regards,
> Vladimir Ivanov
>
> On 11/13/17 7:34 PM, Claes Redestad wrote:
>> Hi,
>>
>>   this patch factors out the BoundMethodHandle species data class
>> generation to a new ClassSpecializer facility.
>>
>>   While currently semantically neutral, this will make it possible
>> to reuse the facility in other places.
>>
>>   Webrev: http://cr.openjdk.java.net/~redestad/8184777/open.00/
>>   Bug: https://bugs.openjdk.java.net/browse/JDK-8184777
>>
>>   Performance wise this adds a very small (~20k bytecode) amount
>> of work to the initialization costs of BMHs, which we expect will
>> be more than repaid as we apply the ClassSpecializer elsewhere.
>>
>>   Thanks!
>>
>> /Claes

Reply | Threaded
Open this post in threaded view
|

RE: 8184777: Factor out species generation logic from BoundMethodHandle

Christoph Dreis
In reply to this post by Claes Redestad
Hey Claes,

though far away from being an expert on the subject matter, I have some very minor comments if you don't mind.

ClassSpecializer.java
L510: * For example, a concrete species for two reference and one integral bound values have a shape like the following:
Should be imho:
L510: * For example, a concrete species for two references and one integral bound value has a shape like the following:

LambdaFormBuffer.java:
L333: if (oldFns.size() == 0)  return this;
Could be:
L333: if (oldFns.isEmpty())  return this;

Cheers,
Christoph

> -----Original Message-----
> From: core-libs-dev [mailto:[hidden email]] On
> Behalf Of Claes Redestad
> Sent: Monday, November 13, 2017 5:35 PM
> To: core-libs-dev <[hidden email]>
> Cc: [hidden email]
> Subject: RFR: 8184777: Factor out species generation logic from
> BoundMethodHandle
>
> Hi,
>
>   this patch factors out the BoundMethodHandle species data class
> generation to a new ClassSpecializer facility.
>
>   While currently semantically neutral, this will make it possible to reuse the
> facility in other places.
>
>   Webrev: http://cr.openjdk.java.net/~redestad/8184777/open.00/
>   Bug: https://bugs.openjdk.java.net/browse/JDK-8184777
>
>   Performance wise this adds a very small (~20k bytecode) amount of work to
> the initialization costs of BMHs, which we expect will be more than repaid as
> we apply the ClassSpecializer elsewhere.
>
>   Thanks!
>
> /Claes

Reply | Threaded
Open this post in threaded view
|

Re: 8184777: Factor out species generation logic from BoundMethodHandle

Claes Redestad
Hi Christoph,

On 2017-11-13 18:56, Christoph Dreis wrote:

> Hey Claes,
>
> though far away from being an expert on the subject matter, I have some very minor comments if you don't mind.
>
> ClassSpecializer.java
> L510: * For example, a concrete species for two reference and one integral bound values have a shape like the following:
> Should be imho:
> L510: * For example, a concrete species for two references and one integral bound value has a shape like the following:
>
> LambdaFormBuffer.java:
> L333: if (oldFns.size() == 0)  return this;
> Could be:
> L333: if (oldFns.isEmpty())  return this;

nice catches! Fixed and updated webrev in-place.

http://cr.openjdk.java.net/~redestad/8184777/open.00/

Thanks!

/Claes
Reply | Threaded
Open this post in threaded view
|

RE: 8184777: Factor out species generation logic from BoundMethodHandle

Christoph Dreis
Hi Claes,

Thanks for incorporating this.

> > ClassSpecializer.java
> > L510: * For example, a concrete species for two reference and one integral
> bound values have a shape like the following:
> > Should be imho:
> > L510: * For example, a concrete species for two references and one
> integral bound value has a shape like the following:
> >
> > LambdaFormBuffer.java:
> > L333: if (oldFns.size() == 0)  return this; Could be:
> > L333: if (oldFns.isEmpty())  return this;
>
> nice catches! Fixed and updated webrev in-place.
>
> http://cr.openjdk.java.net/~redestad/8184777/open.00/

I still think it should be called "one integral bound value" instead of "values" and "has" instead of "have" (at least in case a singular species is meant).
You seem to have fixed the singular "reference" typo only in ClassSpecializer.java.

While looking at ClassSpecializer again, I found two additional editorial things I wanted to raise:

1.)
364          * @return class name, which by default is {@code outer().topClass().getName() + "$Species_" + deriveTypeString(key)}
 365          */
 366         protected String deriveClassName() {
 367             return topClass.getName() + "$Species_" + deriveTypeString();
 368         }

The @return doesn't seem to match the implementation. This looks a bit weird at least.

2.)
328          * and produces a value of the required type.
Should be "produce a value" given that "supply" is used some lines above.

Again - I hope you don't mind these minor comments.

Cheers,
Christoph

Reply | Threaded
Open this post in threaded view
|

Re: 8184777: Factor out species generation logic from BoundMethodHandle

Claes Redestad
Hi Christoph,

On 2017-11-14 15:29, Christoph Dreis wrote:
> I still think it should be called "one integral bound value" instead of "values" and "has" instead of "have" (at least in case a singular species is meant).
> You seem to have fixed the singular "reference" typo only in ClassSpecializer.java.

I read your comment too fast. Fixed!

>
> While looking at ClassSpecializer again, I found two additional editorial things I wanted to raise:
>
> 1.)
> 364          * @return class name, which by default is {@code outer().topClass().getName() + "$Species_" + deriveTypeString(key)}
>   365          */
>   366         protected String deriveClassName() {
>   367             return topClass.getName() + "$Species_" + deriveTypeString();
>   368         }
>
> The @return doesn't seem to match the implementation. This looks a bit weird at least.

Updated code to match documentation.

>
> 2.)
> 328          * and produces a value of the required type.
> Should be "produce a value" given that "supply" is used some lines above.

Fixed!

>
> Again - I hope you don't mind these minor comments.

Don't mind at all. Some of the typos here were pre-existing, and while
this is
internal code, it surely doesn't hurt cleaning up comments.

http://cr.openjdk.java.net/~redestad/8184777/open.01/

Thanks!

/Claes