RFR(L): 8140485: Class load and creation clean up
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Mon Nov 23 12:48:05 UTC 2015
Hi Markus,
These are the comments I have at the moment.
webrev03/src/share/vm/oops/constantPool.cpp
67 static bool tag_array_is_zero_initialized(Array<u1>* tags) {
68 assert(tags != NULL, "invariant");
69 const int length = tags->length();
70 for (int index = 0; index < length; index++) {
71 if (JVM_CONSTANT_Invalid != tags->at(index)) {
72 return false;
73 }
74 }
75 return true;
76 }
77
78 #endif
79
80 ConstantPool::ConstantPool(Array<u1>* tags) :
81 _length(tags->length()),
82 _flags(0) {
83 assert(tags != NULL, "invariant");
84 assert(tags->length() == _length, "invariant");
85 assert(tag_array_is_zero_initialized(tags), "invariant");
86 set_tags(tags);
87
88 // only set to non-zero if constant pool is merged by RedefineClasses
89 set_version(0);
90
91 assert(NULL == _cache, "invariant");
92 assert(NULL == _reference_map, "invariant");
93 assert(NULL == _resolved_references, "invariant");
94 assert(NULL == _operands, "invariant");
95 assert(NULL == _pool_holder, "invariant");
96 // maybe its enough to just check one of the above
97 // fields to verify underlying calloc allocation
98 }
Both checks at the lines 83 ans 69 are late as tags is dereferenced
at the line 81. Would it better to remove the line 81 and initialize the
_length in the 'tag_array_..' after the check at 68?
I'd also suggest to follow the comment at 96-97: to leave just one
of the asserts 91-95 and remove the comment.
webrev03/src/share/vm/oops/arrayKlass.hpp
93 // Casting from Klass*
94 static const ArrayKlass* cast(const Klass* k) {
95 assert(k->is_array_klass(), "cast to ArrayKlass");
96 return static_cast<const ArrayKlass*>(k);
97 }
Minor: The comment at line 93 is not fully correct as the casting is from 'const Klass*'.
The same issue is in some other updated files:
instanceKlass.hpp, objArrayKlass.hpp, typeArrayKlass.hpp
webrev03/src/share/vm/runtime/vmStructs.cpp
Minor: Line 323: Unaligned '\' at the end.
Thanks,
Serguei
On 11/20/15 11:02, Coleen Phillimore wrote:
>
> Hi Markus,
>
> I ran your n-1 change through my local tests and they all passed.
>
> The only thing left is I think adding comments would be good to
> KlassFactory.hpp/cpp.
>
> Thanks,
> Coleen
>
> On 11/20/15 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
>>>
>
More information about the hotspot-runtime-dev
mailing list