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

Fri Nov 20 18:27:57 UTC 2015

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