Review Request: Add ClassOption.STRONG and default is unspecified

[David Holmes]

On 12/03/2020 9:05 am, Mandy Chung wrote:
> On 3/11/20 3:34 PM, David Holmes wrote:
>> Hi Mandy,
>>
>> On 12/03/2020 8:12 am, Mandy Chung wrote:
>>> Webrev:
>>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/weak-strong-class/ 
>>>
>>>
>>> Alex suggests to keep the default unspecified, which I think it's a 
>>> great suggestion such that people do not have to rely on the 
>>> implementation assumption.
>>
>> I think this is making things unnecessarily awkward. The suggestion 
>> from yesterday (as per below) was to change the default to be weak, 
>> and allow people to ask for strong. That would involve simply changing 
>> all WEAK to STRONG through the existing code. Now we have WEAK and 
>> STRONG that are mutually exclusive so have to check for them both 
>> being set, and if neither is set then we fall into 
>> implementation-specific behaviour.
>> If it is insisted that we have both WEAK and STRONG then we can 
>> achieve having the default unspecified by requiring that one of them 
>> always be passed in. 
> 
> There are cases that the library code does not care.

I am skeptical of that. If you do the analysis and determine that you 
don't need a strong connection then actually getting a strong connection 
will work fine but may lead to footprint issues. If you don't need a 
strong connection I don't see why you would choose to let it default 
rather than selecting weak. If you select weak and find there is a 
performance issue then you would switch to strong, comment why, and file 
a RFE to fix the performance issue. I just don't see "don't care" coming 
into this. And of course if you need strong then allowing it to default 
is just broken!

> Note that the main motivation to allow a hidden class be strongly 
> referenced because of our current implementation.   It creates a 
> ClassLoaderData per weak class that may result in fragmentation and 
> incur bad performance overhead.

That sounds like a deficiency in the VM that should be addressed.

> This is solely implementation detail.  Other VM implementation may favor 
> weak classes.  An implementation-specific default will give flexibility 
> to the developers (not to specify neither strong or weak) to choose 
> whatever performs best when running on a VM implementation.

Again I just don't buy that developers will write code that way. No one 
wants to depend on the performance characteristics of a default that 
could change on every release.

David
-----

> Mandy
> 
>> But I much prefer what was suggested previously: either default is 
>> strong and you can ask for weak; or default is weak and you can ask 
>> for strong.
>>
>> Sorry.
>>
>> David
>>
>>> This patch adds a new ClassOption::STRONG enum constant while keeping 
>>> ClassOption::WEAK.
>>
>>> Mandy
>>>
>>> On 3/10/20 9:55 PM, John Rose wrote:
>>>> I agree with Alex.  Class loaders map names to classes.  It’s not
>>>> natural *semantically* for a nameless class to have its lifetime
>>>> determined by a class loader.  It may be more natural *in our
>>>> JVM implementation* (hence more performant, in the *current*
>>>> implementation) to have a non-weak link, but let’s do this only
>>>> if users request it explicitly.
>>>>
>>>> On Mar 10, 2020, at 4:55 PM, Mandy Chung <mandy.chung at oracle.com 
>>>> <mailto:mandy.chung at oracle.com>> wrote:
>>>>>
>>>>> Brian, John, et al,
>>>>>
>>>>> Alex has given his feedback on the weak class feature (attached).
>>>>>
>>>>> Alex recommends to change the default of hidden classes be weak 
>>>>> classes and introduce a ClassOption::STRONG option if it wants to 
>>>>> ensure that the hidden class is not being reclaimed until the class 
>>>>> loader becomes unreachable.  See more details below.
>>>>>
>>>>> We tried to keep a hidden class be a normal class from JVM's point 
>>>>> of view.  Classes in the VM implementation have the same lifecycle 
>>>>> as the defining class loader. In addition, LambdaForms and Nashorn 
>>>>> are the only JDK use of hidden weak classes.  All others such as 
>>>>> lambda metafactory and string concat use hidden (strong) classes as 
>>>>> they are part of the target class' internal implementation and 
>>>>> should be unloaded altogether. Hence strong reference to hidden 
>>>>> classes is the default.
>>>>>
>>>>> What's your recommendation?
>>>>>
>>>>> Mandy
>>>>>
>>>>>
>>>>> -------- Forwarded Message --------
>>>>> Subject:     Weak references to hidden classes
>>>>> Date:     Tue, 10 Mar 2020 16:16:50 -0700
>>>>> From:     Alex Buckley <alex.buckley at oracle.com>
>>>>> Organization:     Oracle Corporation
>>>>> To:     Mandy Chung <mandy.chung at oracle.com>
>>>>>
>>>>>
>>>>>
>>>>> JEP 371 says that a hidden class may be weakly referenced by its 
>>>>> class loader. Which loader? The only loader with any connection to 
>>>>> a hidden class is the loader nominated to serve as the defining 
>>>>> loader by the API spec of defineHiddenClass. However, that spec 
>>>>> doesn't say "The defining loader holds a strong reference to the 
>>>>> hidden class" because no spec says anything about how a class 
>>>>> loader refers to a class.
>>>>>
>>>>> (The only relationship we do specify is the other way around -- 
>>>>> ClassLoader says "Every Class object contains a reference to the 
>>>>> ClassLoader that defined it." -- and happily that relationship 
>>>>> holds for a hidden class too.)
>>>>>
>>>>> Point is, it's odd for defineHiddenClass to standardize how a class 
>>>>> loader refers to a hidden class when, conceptually, class loaders 
>>>>> are not meant to "know" about hidden classes.
>>>>>
>>>>> (The connection between a defining loader and a hidden class is a 
>>>>> mere accounting trick done to support the hidden class's own code. 
>>>>> No class loader, not even that defining loader, knows how to 
>>>>> respond to a class load request for the hidden class.)
>>>>>
>>>>> It's especially odd for defineHiddenClass's default to be a strong 
>>>>> reference. That default aligns with ClassLoader::defineClass, the 
>>>>> API we're trying to avoid, and doesn't align with 
>>>>> Unsafe::defineAnonymousClass, the API we're trying to recreate!
>>>>>
>>>>> I understand there are performance reasons to want a loader to use 
>>>>> a strong reference rather than a weak reference. Accepting that 
>>>>> claim, I recommend having the implementation use a weak reference 
>>>>> by default, having the spec allow that, then introducing 
>>>>> ClassOption.STRONG to force a strong reference. That is, specify that:
>>>>>
>>>>> "In the absence of an option, the defining loader's relationship 
>>>>> with the hidden class is unspecified, and the loader implementation 
>>>>> may use a strong reference or a weak reference to keep track of the 
>>>>> hidden class. If a hidden class is not strongly referenced by the 
>>>>> class loader which is deemed to be its defining loader, then the 
>>>>> hidden class can be unloaded when library code no longer refers to 
>>>>> it. Library developers who wish to prevent the hidden class from 
>>>>> being unloaded must ensure that its Class object is never 
>>>>> reclaimed, or else use the ClassOption.STRONG option."
>>>>>
>>>>> I understand there is a concern that developers would not realize / 
>>>>> be surprised by getting a weak reference as the default 
>>>>> implementation. However, the planet-wide audience for 
>>>>> defineHiddenClass is perhaps 500 developers. They already have to 
>>>>> know what they're doing because the features of hidden classes are 
>>>>> already more limited than the features of VM-anonymous classes., 
>>>>> e.g., no constant pool patching. The usual "Java should save 
>>>>> developers from making silly mistakes" rule doesn't apply here.
>>>>>
>>>>> Alex
>>>>
>>>
>