RFR(L): 8140485: Class load and creation clean up
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Nov 24 09:36:33 UTC 2015
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
>>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20151124/d3cbebf1/attachment-0001.html>
More information about the serviceability-dev
mailing list