RFR JDK-8232222: Set state to 'linked' when an archived class is restored at runtime

[David Holmes]

Hi Jiangli,

On 20/12/2019 1:01 pm, Jiangli Zhou wrote:
> Hi Coleen,
> 
> Thank you for your feedback! I was planning to ping you and get your
> opinion. Happy to see your comment and involvement!
> 
> On Thu, Dec 19, 2019 at 2:26 PM <coleen.phillimore at oracle.com> wrote:
>>
>>
>>
>> On 12/16/19 10:30 PM, Jiangli Zhou wrote:
>>> Since this change involves security related issues, Ioi, Claes and I
>>> had off-mailing list discussion and initial code review (thanks!).
>>> Thanks Ioi for investigating and providing test cases in security
>>> related area. As the security related parts are settled, I'd like to
>>> resume the discussion and review in the open,  and hope to move this
>>> forward.
>>>
>>> My initial change (webrev.00) set all archived classes loaded by
>>> builtin loaders in 'linked' state during restoration in
>>> InstanceKlass::restore_unshareable_info(). Ioi demonstrated a
>>> potential issue for classes loaded by the AppClassLoader (thanks
>>> again). More investigation and work are needed to handle the general
>>> case for AppClassLoader. Following are two proposals provide the
>>> support with narrower scopes with no potential security issue. Option
>>> (1) is an approach that I implemented in one of my earlier patches.
>>>
>>> Option (1)
>>> ========
>>> Set an archived class in 'linked' state during restoration in
>>> InstanceKlass::restore_unshareable_info() for the following cases:
>>>
>>> Case 1): For an archived class loaded by the NULL class loader by
>>> default (when verification is not required for classes loaded by the
>>> NULL class loader, which is the default case).
>>> Case 2): For an archived class loaded by the PlatformClassLoader and
>>> AppClassLoader when verification is disabled at runtime. This use case
>>> (disabling class verification) appears to be important in some cases
>>> for real world applications where verification can be safely disabled.
>>>
>>> http://cr.openjdk.java.net/~jiangli/8232222/webrev.01/
>>
>> Hi Jiangli,
>>
>> We've deprecated -verify:none and I don't want to have the JVM add any
>> feature or an optimization in particular that uses this.  I don't think
>> we should invite these bugs.
> 
> Your feedback sound reasonable for options (1).
> 
> Not completely related to this discussion, I have a feedback/input
> about -Xverify:none removal based on my observation of the flag
> usages. -Xverify:none is useful for reducing performance overhead
> caused by verification and users do enable it when it is safe and
> performance is important. In some cases, there can be ~10% performance
> difference. For -Xverify:none removal, it would be important to have a
> good strategy on how and when to do so, at least after dynamic
> archiving is completely mature, and ready for its prime time and broad
> adoptions.
> 
>>
>>>
>>> Option (2)
>>> ========
>>> Set an archived class loaded by the NULL loader in 'linked' state
>>> during runtime restoration in
>>> InstanceKlass::restore_unshareable_info() by default. Option (2) only
>>> supports the case (1) described in option (1).
>>>
>>> http://cr.openjdk.java.net/~jiangli/8232222/webrev.02/
>>>
>>> Both of the above proposals provide the performance benefit described
>>> in the original review request.
>>>
>>> In addition to the performance benefit, my other motivation for this
>>> change is to provide a foundation for potential optimization with more
>>> general class pre-initialization (currently only a few special cases
>>> are supported in the JDK classes) on top of the heap object graph
>>> archiving mechanism (, and constant pool field and method references
>>> pre-resolution) in the future. Apologize for not clearly explain that
>>> in the original review request, although the RFE was linked to the
>>> umbrella RFE for pre-initialization/pre-resolution. I've clarified it
>>> in the RFE as well.
>>
>> I've read the RFE for this and the umbrella RFE, but I don't understand
>> the design or how this would accomplish it.  Could you have the
>> initialized version of the class in the archive instead and use it when
>> we call klass->initialize() rather than calling into the <clinit> function?
> 
> It is a fine idea. If there is no JVMS compliance issue, we can
> explore in this direction since it could be slightly more efficient
> (not observable). We still need to post JVMTI events for loading,
> preparing, etc at runtime. Can we involve David H. for the Spec
> related discussions?

I'm always lurking :)

I'm not clear what exactly is being proposed at this stage, but I assume 
this is a discussion for a future RFE in the New Year.

Cheers,
David

>>
>> The JEP process is being streamlined more, so maybe this work requires
>> one.  I only just dropped into this thread but I'd love to learn more
>> about it, but after the holidays.  I have some concerns about this code
>> change.  It seems innocuous enough but it's limited to the
>> bootclassloader and I don't know how this will help.
> 
> Ioi and I also briefly discussed about JEP process. Maybe we could
> break the work into a series of JEPs if necessary, so it still enables
> incremental developments? Each self-contained block of work could be
> mapped to an individual JEP. That would help us validate the benefit
> of each sub-work (of pre-resolution and pre-initialization) in real
> world production environment more efficiently, and give the access to
> these new improvements to OpenJDK users in the community more quickly.
> It would also help me streamline the private patches without dealing
> with a lot of code conflicts and reduce the cost/overhead of
> maintaining the patches.
> 
> I'd love to discuss more with you after the holidays. I will also need
> to do some more researches (about the process) on my side.
> 
> Best regards,
> Jiangli
> 
>>
>> Thanks,
>> Coleen
>>>
>>> Following are more details on pre-initialization for interested
>>> readers. I'm still investigating this area and flushing out the
>>> details. So the best approach is to do those incrementally. The
>>> information that I provide below may change. It is probably better to
>>> have a general pre-initialization and pre-resolving discussion that's
>>> separate from this review request. And we can move the current change
>>> forward once it's reviewed and accepted.
>>>
>>> One immediate improvement that the current change would enable is
>>> simplifying the existing static field pre-initialization applied to
>>> some of the JDK classes. When an archived class can be set in the
>>> 'linked' state during runtime restoration, it makes it possible to
>>> place the class in 'initialized' state at restoration. As loading,
>>> verifying, linking/preparing and initializing orders are mandated by
>>> JVMS, a class can only be placed in 'initialized' after it is already
>>> in 'linked' state. Also, we want to avoid a partially initialized
>>> class state. Currently when static fields for a given JDK class is
>>> pre-initialized at CDS dump time, we need to store the preserved field
>>> values separately from the class mirror object (j.l.Class instance).
>>> At runtime, during running the class' static initializer (<clinit>),
>>> the VM finds and stores the values back to the corresponding static
>>> fields in the mirror.  Following is an example where we pre-initialize
>>> java.lang.module.Configuration. In the code below,
>>> VM.initializeFromArchive() is called to retrieve the preserved value
>>> and store back to the mirror. A check is performed to verify if the
>>> value is restored from archive, otherwise we do runtime
>>> initialization. The complication is due to the separate storage for
>>> the preserved values (stored outside the mirror).
>>>
>>>       static {
>>>           // Initialize EMPTY_CONFIGURATION from the archive.
>>>           VM.initializeFromArchive(Configuration.class);
>>>
>>>           // Create a new empty Configuration if there is no archived version.
>>>           if (EMPTY_CONFIGURATION == null) {
>>>               EMPTY_CONFIGURATION = new Configuration();
>>>           }
>>>       }
>>>
>>> With the proposed change in the current review request, it would
>>> enable us to set Configuration in 'initialized' state during runtime
>>> restoration, and therefore allow us to preserve the static field value
>>> as part of the mirror without violating the mandated ordering
>>> requirement. With that, we can eliminate the
>>> VM.initializeFromArchive() call and the if check in the Java code for
>>> some of the use cases. Once Configuration is fully 'restored' at
>>> runtime, there is no need to execute the <clinit> at all. It would
>>> allow us to apply pre-initialization to more JDK classes if desired,
>>> without complicating the Java code.
>>>
>>> When we can properly set an archived class to 'initialized' state at
>>> restore time, it then will enable us to pre-resolve related constant
>>> pool method and field references.
>>>
>>> Ioi had some questions for pre-initialization and pre-resolution which
>>> I'm including below with my answers inlined. As I'm still
>>> investigating this area, so most likely they are not final.
>>>
>>> - Are you going to do the same thing for the initialized state -- all
>>> classes loaded by boot loader will be automatically initialized?
>>>
>>> By default, an archived class loaded by the boot loader will only be
>>> placed in 'linked' state at restoration time when it is suitable. For
>>> 'value' object that cannot be placed in 'linked' state at restore time
>>> when archiving supported for that is added in the future, it can be
>>> handled differently and the related class can remain in 'alloced'
>>> state during restoration.
>>>
>>> An archived class will only be set to 'initialized' during runtime
>>> restoration if the class can be in fully 'initialized' state with all
>>> static fields pre-initialized and preserved. An archived class can be
>>> 'flagged' for that case at dump time. Specific implementation details
>>> need further investigation.
>>>
>>> - That certainly doesn't seem feasible, so you will probably need to
>>> put some restrictions. So why aren't you putting in those restrictions
>>> now? Do you know what these restrictions are?
>>>
>>> The restriction only applies when we add support for more general
>>> 'pre-initialization' and 'value' object archiving. Currently, for the
>>> restore-time 'linked' state implementation in the webrevs, the
>>> restriction does not apply.
>>>
>>> - Will they be so restrictive that this will not be practical?
>>>
>>> As pre-initialization cannot be applied to all cases, selective and
>>> targeted pre-initialization (following current CDS static field
>>> pre-initializing model) is a more flexible approach that provides both
>>> performance benefits and JVMS/language spec compliant.
>>>
>>> Comments are welcome and appreciated!
>>>
>>> Best regards,
>>> Jiangli
>>>
>>>
>>>
>>>
>>> On Thu, Nov 28, 2019 at 9:33 AM Jiangli Zhou <jianglizhou at google.com> wrote:
>>>> Hi,
>>>>
>>>> Please review the following optimization related to archived classes'
>>>> (from builtin loaders) runtime restoration and linking. It is not
>>>> intended for OpenJDK 14. After reviewers review the change, I'll wait
>>>> for 14 fork before pushing.
>>>>
>>>> webrev: http://cr.openjdk.java.net/~jiangli/8232222/webrev.00/
>>>> RFE: https://bugs.openjdk.java.net/browse/JDK-8232222
>>>>
>>>> Motivation and details of the change, which are duplicated in the RFE
>>>> =====================================================
>>>>
>>>> When linking a class, InstanceKlass::link_class_impl() first links all
>>>> super classes and super interfaces of the current class. For the
>>>> current class, it then verifies and rewrites the bytecode, links
>>>> methods, initializes the itable and vtable, and sets the current class
>>>> to 'linked' state.
>>>>
>>>> When loading an archived class at runtime,
>>>> SystemDictionary::load_shared_class makes sure the super types (all
>>>> super classes and super interfaces) in the class hierarchy are loaded
>>>> first. If not, the archived class is not used. The archived class is
>>>> restored when 'loading' from the archive. At the end of the
>>>> restoration, all methods are linked. As bytecode verification and
>>>> rewriting are done at CDS dump time, the runtime does not redo the
>>>> operations for an archived class.
>>>>
>>>> If we make sure the itable and vtable are properly initialized (not
>>>> needed for classes loaded by the NULL class loader) and
>>>> SystemDictionaryShared::check_verification_constraints is performed
>>>> for an archived class during restoration, then the archived class
>>>> (from builtin loaders) is effectively in 'linked' state.
>>>>
>>>> For all archived classes loaded by the builtin loaders, we can safely
>>>> set to 'linked' state at the end of restoration. As a result, we can
>>>> save the work for iterating the super types in
>>>> InstanceKlass::link_class_impl() at runtime.
>>>>
>>>> Performance results
>>>> ================
>>>>
>>>> With both JDK 11 and the latest jdk/jdk, the proposed change saves
>>>> ~1.5M instruction execution when running HelloWorld with the default
>>>> CDS. Please see raw data in the RFE. For applications using more
>>>> archived classes at runtime, larger saving should be experienced.
>>>>
>>>> Testing
>>>> ======
>>>> Tested with all jtreg cds/* tests, which include all appcds tests.
>>>> Submit repo testing passed.
>>>>
>>>> The change has also gone through internal testing with very large
>>>> number of tests (all with default CDS enabled) for more than a month.
>>>>
>>>> Best,
>>>> Jiangli
>>