Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

> On 4 Jun 2020, at 21:16, Mandy Chung wrote: > > ... > I finalized an updated CSR per all feedbacks which includes the deprecation > `sun.misc.Unsafe::shouldBeInitialized`. >https://bugs.openjdk.java.net/browse/JDK-8245871 > > updated webrev: >

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

I create https://bugs.openjdk.java.net/browse/JDK-8246634 to track this. thanks Mandy On 6/4/20 4:35 PM, John Rose wrote: P.S.C. (post-send clarification) The workflow would be: static final MethodHandle MH_ensureInit = publicLookup().findVirtual(L…, “ensureInitialized”…); By that I

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

P.S.C. (post-send clarification) > The workflow would be: > > static final MethodHandle MH_ensureInit > = publicLookup().findVirtual(L…, “ensureInitialized”…); > By that I mean the workflow of the dynamic language implementor. And after hitting “send” I realized that optimizing that one

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

On Jun 3, 2020, at 7:38 PM, Mandy Chung wrote: > > On 6/3/20 6:24 PM, John Rose wrote: >> On Jun 3, 2020, at 5:16 PM, Paul Sandoz > > wrote: >>> >>> if (UNSAFE.shouldBeInitialized(cls)) { >>> UNSAFE.ensureClassInitialized(cls); >>> } >>> >>> Although it

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

On Jun 4, 2020, at 12:39 PM, mark.reinh...@oracle.com wrote: > > I agree that the `ensure` prefix is useful here. > > This method can only ensure that a class is initialized, so including > `Class` in the method name seems redundant. I’d go with the shorter > `ensureInitialized`. +1

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

On 6/4/20 12:39 PM, mark.reinh...@oracle.com wrote: I agree that the `ensure` prefix is useful here. This method can only ensure that a class is initialized, so including `Class` in the method name seems redundant. I’d go with the shorter `ensureInitialized`. - Mark Thanks for the advice.  

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

> On Jun 4, 2020, at 12:31 PM, Mandy Chung wrote: > > JVMS 5.5 is the spec for class initialization. Step 3 describes the > recursive request for initialization. > Ok. (From quick offline chat we agreed its not worth explicitly highlighting this case.) > I have @jvms 5.5 in the javadoc.

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

2020/6/3 19:31:13 -0700, mandy.ch...@oracle.com: > On 6/3/20 5:16 PM, Paul Sandoz wrote: >> Did you consider an alternative name, such as: >> >> /** >> * Initializes {@code targetClass}, if not already initialized. >> * ... >> */ >> public Class initializeClass(Class targetClass) ... >> >> ? >

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

Lookup::accessClass sets the precedence to return Class for method chaining.  So this new method follows the existing convention. Mandy On 6/4/20 12:31 PM, Florent Guillaume wrote: Hi, Why can't it just return void? Florent On Thu, Jun 4, 2020 at 9:22 PM Mandy Chung

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

Hi, Why can't it just return void? Florent On Thu, Jun 4, 2020 at 9:22 PM Mandy Chung wrote: > > > On 6/4/20 9:43 AM, Chris Hegarty wrote: > > Mandy, > > > > I think this looks good. Just a few minor comments... > > > > The spec wording is hard, since the method is offering a conditional >

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

JVMS 5.5 is the spec for class initialization.  Step 3 describes the recursive request for initialization. I have @jvms 5.5 in the javadoc. I can make it explicit: * Ensures that {@code targetClass} has been initialized. The class * to be initialized must be {@linkplain

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

On 6/4/20 9:43 AM, Chris Hegarty wrote: Mandy, I think this looks good. Just a few minor comments... The spec wording is hard, since the method is offering a conditional initialization. I guess I expected to see something like “initializes targetClass, if it has not been initialized

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

Hi Mandy, What about this case: class Test { static { MethodHandles.lookup().ensureClassInitialized(Test.class); // not yet initialized } } (I see you do that for m/p1/A.java and Test.clinitInvokeEnsureClassInitialized.) Do we need mention this in the spec? e.g. this method does

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

> On Jun 4, 2020, at 11:43 AM, Alan Bateman wrote: > > On 04/06/2020 17:55, Paul Sandoz wrote: >> : >> FWIW I keep tripping over the prefix “ensures”. As I understand the >> implementation initializes a class, if not already so (and it has to check >> as you point out below). Focusing on

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

On 04/06/2020 17:55, Paul Sandoz wrote: : FWIW I keep tripping over the prefix “ensures”. As I understand the implementation initializes a class, if not already so (and it has to check as you point out below). Focusing on the main action seems appropriate in this regard. I expect John has

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

> On Jun 3, 2020, at 7:31 PM, Mandy Chung wrote: > > > > On 6/3/20 5:16 PM, Paul Sandoz wrote: >> Hi Mandy, >> >> Did you consider an alternative name, such as: >> >> /** >> * Initializes {@code targetClass}, if not already initialized. >> * ... >> */ >> public Class

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

Mandy, I think this looks good. Just a few minor comments... The spec wording is hard, since the method is offering a conditional initialization. I guess I expected to see something like “initializes targetClass, if it has not been initialized already”, or some such. The input and output is

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

On 6/3/20 6:24 PM, John Rose wrote: On Jun 3, 2020, at 5:16 PM, Paul Sandoz > wrote:  if (UNSAFE.shouldBeInitialized(cls)) {  UNSAFE.ensureClassInitialized(cls);  } Although it seems redundant to perform the check, suggesting it is not needed. It’s

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

On 6/3/20 5:16 PM, Paul Sandoz wrote: Hi Mandy, Did you consider an alternative name, such as: /** * Initializes {@code targetClass}, if not already initialized. * ... */ public Class initializeClass(Class targetClass) ... ? I considered this.   The targetClass may have been

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

On Jun 3, 2020, at 5:16 PM, Paul Sandoz wrote: > > if (UNSAFE.shouldBeInitialized(cls)) { > UNSAFE.ensureClassInitialized(cls); > } > > Although it seems redundant to perform the check, suggesting it is not needed. It’s useful (IIRC) in this kind of code: MethodHandle mh = … something

Re: Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

Hi Mandy, Did you consider an alternative name, such as: /** * Initializes {@code targetClass}, if not already initialized. * ... */ public Class initializeClass(Class targetClass) ... ? What about the companion Unsafe.shouldBeInitialized? I have seen (and written) code like this: if

Review Request: JDK-8235521: Replacement API for Unsafe::ensureClassInitialized

This proposes a new `Lookup::ensureClassInitialized` API as a replacement for `sun.misc.Unsafe::ensureClassInitialized`.The Lookup object must have the access to the given class being initialized. CSR: https://bugs.openjdk.java.net/browse/JDK-8245871 webrev: