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

[coleen.phillimore at oracle.com]

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.

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

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.

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