[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR(L): 8140485: Class load and creation clean up
From: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2015-11-30 20:37:22
Message-ID: 565CB382.4080303 () oracle ! com
[Download RAW message or body]
Hi Markus,
On 11/26/15 04:20, Markus Gronlund wrote:
>
> Hi Serguei and Coleen,
>
> Many thanks again for traversing this change.
>
> Serguei,
>
> I have updated to address (some of) your comments:
>
>
> “ 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.”
>
> In general it’s hard to assert input parameters when an initialization
> list is used – here the assert is more of an expressed invariant
> (communication aspect), than a “trap before use” assertion.
>
> I like to use an initialization list instead of assignments
> (constructor body) for this code. Based on your input I have updated
> some of the assertions and removed the original comment I put in there
> – thanks.
>
I'm Ok with the initialization list.
Thank you for the change.
> 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
>
> I have removed the “Casting from…” comment(s) that was used in
> contexts similar to:
>
> // Casting from Klass*
>
> staticArrayKlass* cast(Klass* k) {
>
> This comment is a bit redundant – thanks.
>
> I have also fixed the indentation issue in vmstructs – thanks.
>
Thanks for the update!
The fix looks good modulo last comments from Coleen posted on 11/24.
Thanks,
Serguei
> Coleen,
>
> I have tried to put in a module/class/function summary comment in
> klassFactory.hpp – thanks.
>
> Updated webrev:
>
> http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/webrev04/
> <http://cr.openjdk.java.net/%7Emgronlun/8140485/unified/unified_review/webrev04/>
>
> Thanks again for your help
>
> Markus
>
> *From:*Serguei Spitsyn
> *Sent:* den 24 november 2015 13:42
> *To:* Markus Gronlund
> *Cc:* serviceability-dev; Coleen Phillimore;
> hotspot-runtime-dev@openjdk.java.net
> *Subject:* Re: RFR(L): 8140485: Class load and creation clean up
>
> 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@oracle.com
> <mailto:serguei.spitsyn@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@oracle.com
> <mailto:serguei.spitsyn@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/
> <http://cr.openjdk.java.net/%7Emgronlun/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@openjdk.java.net
> <mailto:hotspot-runtime-dev@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
> <http://cr.openjdk.java.net/%7Emgronlun/8140485/unified/unified_review/81>
>
> 40485_updated.jpg
>
> Updated webrev:
> http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/we
> <http://cr.openjdk.java.net/%7Emgronlun/8140485/unified/unified_review/we>
>
> brev02/
>
> Updated patch:
> http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/81
> <http://cr.openjdk.java.net/%7Emgronlun/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@openjdk.java.net
> <mailto:hotspot-runtime-dev@openjdk.java.net>;
> serviceability-dev@openjdk.net
> <mailto:serviceability-dev@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
> <http://cr.openjdk.java.net/%7Emgronlun/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/
> <http://cr.openjdk.java.net/%7Emgronlun/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/
> <http://cr.openjdk.java.net/%7Emgronlun/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/
> <http://cr.openjdk.java.net/%7Emgronlun/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/
> <http://cr.openjdk.java.net/%7Emgronlun/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/
> <http://cr.openjdk.java.net/%7Emgronlun/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/
> <http://cr.openjdk.java.net/%7Emgronlun/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/
> <http://cr.openjdk.java.net/%7Emgronlun/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/
> <http://cr.openjdk.java.net/%7Emgronlun/8140485/unified/webrev01/>
>
>
> Patch:
> http://cr.openjdk.java.net/~mgronlun/8140485/unified/8140485_unified.p
> <http://cr.openjdk.java.net/%7Emgronlun/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
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic