Review Request: Add ClassOption.STRONG and default is unspecified

[Mandy Chung]

On 3/11/20 7:26 PM, David Holmes wrote:
> 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!
>

I consider LambdaMetaFactory and StringConcatFactory be indifferent to 
the implementation details because the resolved BSM is a CallSite 
pointing to a hidden class, i.e. keep the hidden class alive anyway.

Are you saying this is wrong for LMF and SCF not to make a deliberate 
decision whether the hidden class is strong or weak?  Put it in another 
way, if we chose a default for them, the library developers are forced 
to consider the performance difference and any other implication between 
strong and weak - which is what we should avoid.

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

I had similar first reaction when I learned that.   This is 
implementation detail and specifying the default be unspecified and let 
the implementation to choose the performant one as the default allows it 
to switch to different default in different release with no cost on 
existing code.

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

Please share use cases, if any.

Mandy

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