[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(L): 8140485: Class load and creation clean up
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2015-10-28 1:16:36
Message-ID: 563021F4.60501 () oracle ! com
[Download RAW message or body]

Hi Markus,

I like the general direction of this refactoring but did not go through 
all the changes yet.
One quick comment though.

It'd make sense to introduce the classPathEntry.cpp matching the 
classPathEntry.hpp header.

Thanks,
Serguei


On 10/27/15 05:20, Markus Gronlund wrote:
> 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_creation_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.patch
> 
> 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
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]