RFR(L): 8140485: Class load and creation clean up
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Nov 20 03:37:37 UTC 2015
Hi Markus,
This is massive but fortunately I've already looked at the incremental
changes so it makes sense in that context.
I love all the new const's! Thank you following through with these.
I have some requested name changes:
- instanceKlassHandle k = ClassFileParser(st).parseClassFile(class_name,
+ instanceKlassHandle k = ClassFactory::create(st,
+ class_name,
loader_data,
protection_domain,
host_klass,
cp_patches,
- parsed_name,
- true,
THREAD);
I'm having trouble getting my head around this ClassFactory::create
name. It's more like ClassMetadataFactory and create should be
create_from_stream. Then it implies that it parses the stream I
think. ClassFactory seems too overloaded. I like that it's been
moved to a new file away from both systemDictionary and classFileParser
and classLoader.
There's a strange relationship between InstanceKlass and
ClassFileParser. It seems like eventually
ClassFileParser::fill_instance() could be done from the InstanceKlass
constructor, but it calls things in ClassFileParser. What you have is an
improvement.
Can you rename ClassFileParser::instance_klass() function to be
some_verb_instance_klass, like create_and_initialize_instance_klass (too
long). It's too hard to find without an IDE, and can you move it in the
file to just after fill_instance? They should be together.
In instance_klass function, ik will never be NULL because of the check
macro above it.
if (ik != NULL) {
fill_instance(ik, CHECK_NULL);
}
I am running my tests on this and looking more at ClassFileParser for
little things but for the most part, I don't even see any formatting
nits. It looks really nice to me.
This ClassFileParser cleanup is long overdue, and yeah, there's more
that can be done, but this is a very good change.
Thanks,
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/8140485_updated.jpg
>
> Updated webrev:
> http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/webrev02/
>
> Updated patch:
> http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/8140485_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_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
>
>
More information about the hotspot-runtime-dev
mailing list