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-...@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-...@openjdk.java.net; > serviceability-...@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 > >
