RFR(L): 8140485: Class load and creation clean up

Tue Dec 1 21:48:33 UTC 2015

Hi Markus,

The restructuring looks good!

I just have a few minor comments.

1. In classLoader.cpp could you add comments explaining why 
no_verification is specified to the ClassFileStream constructor?

2. Could you replace "invariant" with a more interesting message where 
appropriate in some of the asserts?

3.  In reflection.cpp, could you replace the switch statement in 
get_mirror_from_signature() with an if/else statement?

Thanks! Harold

On 11/20/2015 1:27 PM, Markus Gronlund wrote:
> Hi Coleen,
>
> Many, many thanks for taking on this large change.
>
> Thanks for the feedback, I have updated like this:
>
> ClassFactory::create(...) -> KlassFactory::create_from_stream(...)
>
> // "Klass" better signifies internal representation (metadata) compared to "Class" which has a more classfile contextual association. Fixed.
> // In addition, I have removed all overloaded create_from_stream() functions, reducing them to a single create_from_stream(). The caller will
> // have to pass NULL on unused parameters with this tradeoff.
>
> // renaming
> ClassFileParser::instance_klass(...) -> ClassFileParser::create_instance_klass(...)
>
> ClassFileParser::fill_instance(...) -> ClassFileParser::fill_instance_klass(...)
>
> // moved the parse_stream() and post_process_stream() into the ClassFileParser constructor (process() removed) - thanks!
>
> Here is an updated webrev:
>
> http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/webrev03/
>
> Many thanks again
> Markus
>
>
> -----Original Message-----
> From: Coleen Phillimore
> Sent: den 20 november 2015 04:51
> To: hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(L): 8140485: Class load and creation clean up
>
>
> Hi, More comments on classFileParser.
>
> It seems like the ClassFileParser::process is unnecessary as a
> function.   I think the 3 lines should be imported into
> ClassFileParser::ClassFileParser and it makes sense there.
>
>> Summary:
>>
>> Reduce the visibility of CFP impl methods by internal linkage instead of external linkage.
>>
>> Comment:
>>
>> In my opinion, we should attempt to break the pattern of having private functions declared in header files when the private function does not need to reach inside "this".
> Yes, I agree and appreciate moving all of these functions static and internal in CFP.
>
> Coleen
>
>
> On 11/12/15 7:08 AM, Markus Gronlund wrote:
>> Hi again,
>>
>> I have reworked and simplified this clean up/refactoring suggestion:
>>
>> Updated high-level description:
>> http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/81
>> 40485_updated.jpg
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/we
>> brev02/
>>
>> Updated patch:
>> http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/81
>> 40485_open_unified_updated.patch
>>
>> Thanks in advance
>> Markus
>>
>>
>> -----Original Message-----
>> From: Markus Gronlund
>> Sent: den 27 oktober 2015 13:21
>> To: hotspot-runtime-dev at openjdk.java.net;
>> serviceability-dev at openjdk.net
>> Subject: RFR(L): 8140485: Class load and creation clean up
>>
>> Greetings,
>>
>>    
>>
>> I have spent some time refactoring and cleaning up parts of the class load and creation sequence and code.
>>
>>    
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8140485
>>
>> Overview:
>>
>> There are quite a lot of changes here - hopefully it will not be a deterrent of taking this on- first of all, a lot of changes are mechanical and applicable to "form" more than content- for example, variable, parameter and member function "constness". Same applies to other "domino changes", for example when moving an included header from a .hpp file (maybe replacing with a fwd decl instead).
>>
>>    
>>
>> I have tried to stage the changes for easier overview / review (including a high level diagram). The patches/webrevs are split into a patch series of 7 patches (there is also a unified patch).
>>
>>    
>>
>> Use case:
>>
>> My initial use case for starting all of this clean up / refactoring work is a need to reuse mechanics of the class parsing and creation sequence.
>>
>>    
>>
>> Cleanup / refactoring objectives:
>>
>>    
>>
>> -          Allow for class parsing implementation reuse
>>
>> -          Decouple class parsing and Klass creation
>>
>> -          Decouple JVMTI class file load hook from ClassFileParser
>>
>> -          Add compiler assisted read-only intent in order to assist future refactoring attempts
>>
>> -          "componentify" ClassFileParser  / apply to Klass/IK/AK creations
>>
>> -          Take advantage of some optimizations opportunities (constructors and underlying "calloc" memory)
>>
>> -          Maintain class load performance
>>
>>    
>>
>> High level diagram explaining the updated suggested sequence (high level):
>>
>> http://cr.openjdk.java.net/~mgronlun/8140485/8140485_Class_load_and_cr
>> eation_cleanup_high_level_sequence.jpg
>>
>>    
>>
>> Split webrevs/patches for easier overview / review (do note that not all of these splits will compile individually - there is a unified patch link that includes everything, pls see below):
>>
>>    
>>
>> Sequence: "first" (order of changes)
>>
>> Name: "8140485_1_unified entry point"
>>
>> Link:
>> http://cr.openjdk.java.net/~mgronlun/8140485/split/first/webrev01/
>>
>> Summary:
>>
>> Split out JVMTI agent CFLH from CFP - moved to SD::create_vm_representation_prologue().
>>
>> Channel all class loading via a unified entry point -
>> SD::create_vm_representation()
>>
>> Split out ClassPathEntry into own header.
>>
>> Creation of new class ClassPathFileStream (specialized ClassPathStream) to retain ClassLoaderExt::Context functionality in the refactored code.
>>
>> "Need verify" property is carried within the CFS itself.
>>
>> Comment:
>>
>> SystemDictionary::load_instance_class() will now have a call to
>> "load_class_from_classpath" - the relevance of this name might need to
>> be modified (modules etc?)
>>
>>    
>>
>> Sequence: "second"
>>
>> Name: "8140485_2_read_only_class_file_stream"
>>
>> Link:
>> http://cr.openjdk.java.net/~mgronlun/8140485/split/second/webrev01/
>>
>> Summary:
>>
>> The underlying bytestream (buffer) in a CFS representation should not be altered (advertently or inadvertently) once created, made read-only.
>>
>> Comment:
>>
>> Modifications should work with copies.
>>
>>    
>>
>> Sequence: "third"
>>
>> Name: "8140485_3_componentify_class_file_parser"
>>
>> Link:
>> http://cr.openjdk.java.net/~mgronlun/8140485/split/third/webrev01/
>>
>> Summary:
>>
>> In order to allow for code refactoring of CFP - stack allocated variables are modified into  fields. The entire CFP acts as a generic RAII class for the allocated structures (note CHECK macros).
>>
>> I have not fulfilled refactoring of some of the longest methods since it's is hard to really understand the impact of the inlining I guess we are after here..
>>
>> The performance work done so far indicated improvements in still passing parameters instead of reading the fields directly (so you might still see "fat" methods taking a lot of parameters even though the fields are set).
>>
>>
>> "Consts everywhere.."
>>
>> One of the real hard issues trying to refactor this code is the lack of expressed read-only intent - it's hard to determine (both on reading but more importantly when compiling) what can/should change.
>>
>> Yes, I have entered the "const" rabbit hole related to this code (in some cases it might even be termed gratuitous (event 'consting' function parameter pointers in some cases for example)) - hopefully this will help in upcoming attempts to refactor/simplify this code.
>>
>>    
>>
>> Accessors - CFP becomes a data container to be used in constructors
>> (see change seven). In addition, you will now have a possibility to
>> fetch data from a parser without creating a fullblown Klass
>>
>>    
>>
>> Comment:
>>
>> I think the code could be improved and more refactoring could be
>> accomplished by introducing a Metaspace allocation RAII class - this
>> would allow the functions to decouple even more that is currently
>> (might be next steps)
>>
>>    
>>
>> Sequence: "fourth"
>>
>> Name: "8140485_4_hide_impl_details_class_file_parser"
>>
>> Link:
>> http://cr.openjdk.java.net/~mgronlun/8140485/split/fourth/webrev01/
>>
>> Summary:
>>
>> Reduce the visibility of CFP impl methods by internal linkage instead of external linkage.
>>
>> Comment:
>>
>> In my opinion, we should attempt to break the pattern of having private functions declared in header files when the private function does not need to reach inside "this".
>>
>> (that is, it can use an external interface alt. might not need access
>> at all (helpers))
>>
>>    
>>
>> Sequence: "fifth"
>>
>> Name: "8140485_5_update_entry_point"
>>
>> Link:
>> http://cr.openjdk.java.net/~mgronlun/8140485/split/fifth/webrev01/
>>
>> Summary:
>>
>> Update signatures. Remove the parameter passing where not needed
>> ("parsed_name" reference for example)
>>
>>    
>>
>> Sequence: "sixth"
>>
>> Name:  "8140485_6_types_const_includes_repercussions"
>>
>> Link:
>> http://cr.openjdk.java.net/~mgronlun/8140485/split/sixth/webrev01/
>>
>> Summary:
>>
>> Rippling effects of stronger constness.
>>
>> Forward includes where can be applied. This revealed other code modules not having includes -added.
>>
>> Downcasting of const qualified types (applied Scott Meyers idiom of
>> dual const_cast additions (see Effective C++))
>>
>>    
>>
>> Sequence: "seventh"
>>
>> Name: "8140485_7_applied_use_in_constructors_and_some_optimizations"
>>
>> Link:
>> http://cr.openjdk.java.net/~mgronlun/8140485/split/seventh/webrev01/
>>
>> Summary:
>>
>> Take advantage of above modifications to gain direct upshots in other areas. Use CFP as the data container when constructing Klass/IK/AK.
>>
>> Optimizations: Reduce/optimize Klass/InstanceKlass/ArrayKlass/ConstantPool constructors by using underlying invariant on "calloc" equivalent MetaspaceObj allocations.
>>
>>    
>>
>> Unified change/patch:
>>
>> Webrev: http://cr.openjdk.java.net/~mgronlun/8140485/unified/webrev01/
>>
>> Patch:
>> http://cr.openjdk.java.net/~mgronlun/8140485/unified/8140485_unified.p
>> atch
>>
>> Summary:
>>
>> Unified (folded) patch for easier application/imports, for
>> compilation/try out
>>
>>    
>>
>> Testing:
>>
>> I have done targeted testing during development especially using the
>> "runtime/ParallelClassLoading" test suite in order to ensure
>> functional equivalency. JPRT testing. Performance tests (see below)
>>
>>    
>>
>> Performance:
>>
>> Unstructured:
>>
>> I have used Java Flight Recorder (JFR), which envelops the class loading sequence with high resolution timers - I have used this information to ensure  performance has been stable regarding the changes.
>>
>>    
>>
>> Structured:
>>
>> Startup/footprint-benchmarks (Linux32, Linux64, Windows32, Windows64,
>> Solaris SPARC-T)
>>
>>    
>>
>> It is hard to read anything decisive around these (the effects most
>> likely hides in noise), but at least determine that the changes do not
>> introduce anything statistically significant (+/-) (both time and
>> space)
>>
>> Thanks in advance
>>
>> Markus
>>
>>    