RFR(L): 8140485: Class load and creation clean up
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Nov 24 12:42:10 UTC 2015
Markus,
Sorry, this review is pretty late.
I've passed through the rest of the file classFileParcer.cpp.
It is a thumbs up from me modulo minor comments in the first email.
In general, it is a great shift in the right direction (I mean the
refactoring).
Not sure though, it is possible to use const modifiers consistently,
especially the after type const modifiers. :)
Thanks,
Serguei
On 11/24/15 01:36, serguei.spitsyn at oracle.com wrote:
> Markus,
>
>
> I have reviewed almost all the code.
> It looks good so far.
>
> However, I've got lost at the end of the file classFileParser.cpp
> that includes the functions ClassFileParser::fill_instance_klass(),
> ClassFileParser::ClassFileParser(), etc.
>
> Apparently, it is very hard to make sure everything is right as there are
> many changes there including newly written code but the mapping
> between old and new code is lost in the all diff formats including frames.
>
> Do you want me to continue to review this part, or you have already
> good review coverage for this file? Do I have any extra time for this?
>
> One issue I have is that I'm not sure if the staged webrevs from the
> 1-st review round can
> still be used to help with this or they became obsolete in the 2-nd
> and 3-rd round changes.
>
>
> Thanks,
> Serguei
>
>
>
> On 11/23/15 04:48, serguei.spitsyn at oracle.com wrote:
>> 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