[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-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
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Markus,<br>
<br>
<br>
On 11/26/15 04:20, Markus Gronlund wrote:<br>
</div>
<blockquote cite="mid:547c58b1-ce2c-4d5e-b823-11e01ef24a3a@default"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<meta name="Generator" content="Microsoft Word 12 (filtered
medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0cm;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
span.changed
{mso-style-name:changed;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
span.new
{mso-style-name:new;}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:70.85pt 70.85pt 70.85pt 70.85pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
<div class="WordSection1">
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Hi
Serguei and Coleen,<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> \
</o:p></span></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US">Many thanks again for traversing this \
change.<o:p></o:p></span></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US">Serguei,<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US">I have updated to address (some of) your
comments:<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><br>
“ 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?<br>
I'd also suggest to follow the comment at 96-97: to leave
just one of the asserts 91-95 and remove the \
comment.”<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US"><o:p> \
</o:p></span></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US">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.<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US">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.</span></p>
</div>
</blockquote>
<br>
I'm Ok with the initialization list.<br>
Thank you for the change.<br>
<br>
<br>
<blockquote cite="mid:547c58b1-ce2c-4d5e-b823-11e01ef24a3a@default"
type="cite">
<div class="WordSection1">
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<pre><span lang="EN-US"> Minor: The comment at line 93 is not fully correct \
as the casting is from 'const Klass*'.<o:p></o:p></span></pre>
<pre><span lang="EN-US"> The same issue is in some other updated \
files:<o:p></o:p></span></pre>
<pre><span lang="EN-US"> instanceKlass.hpp, objArrayKlass.hpp, \
typeArrayKlass.hpp<o:p></o:p></span></pre> <p class="MsoNormal"><span \
lang="EN-US"><o:p> </o:p></span></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US">I have removed the “Casting from…” comment(s)
that was used in contexts similar to:<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal" style="text-autospace:none"><span
style="font-size:9.5pt;font-family:Consolas;background:white;mso-highlight:white"
lang="EN-US"> </span><span
style="font-size:9.5pt;font-family:Consolas;color:green;background:white;mso-highlight:white"
lang="EN-US">// Casting from Klass*</span><span
style="font-size:9.5pt;font-family:Consolas;background:white;mso-highlight:white"
lang="EN-US"><o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:9.5pt;font-family:Consolas;background:white;mso-highlight:white"
lang="EN-US"> </span><span
style="font-size:9.5pt;font-family:Consolas;color:blue;background:white;mso-highlight:white"
lang="EN-US">static</span><span
style="font-size:9.5pt;font-family:Consolas;background:white;mso-highlight:white"
lang="EN-US"> </span><span
style="font-size:9.5pt;font-family:Consolas;color:#2B91AF;background:white;mso-highlight:white"
lang="EN-US">ArrayKlass</span><span
style="font-size:9.5pt;font-family:Consolas;background:white;mso-highlight:white"
lang="EN-US">* cast(</span><span
style="font-size:9.5pt;font-family:Consolas;color:#2B91AF;background:white;mso-highlight:white"
lang="EN-US">Klass</span><span
style="font-size:9.5pt;font-family:Consolas;background:white;mso-highlight:white"
lang="EN-US">* </span><span
style="font-size:9.5pt;font-family:Consolas;color:gray;background:white;mso-highlight:white"
lang="EN-US">k</span><span
style="font-size:9.5pt;font-family:Consolas;background:white;mso-highlight:white"
lang="EN-US">) {</span><span
style="font-size:9.5pt;font-family:Consolas" \
lang="EN-US"><o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US"><o:p> \
</o:p></span></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US">This comment is a bit redundant – \
thanks.<o:p></o:p></span></p> <p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US">I have also fixed the indentation issue in
vmstructs – thanks.</span></p>
</div>
</blockquote>
<br>
Thanks for the update!<br>
<br>
The fix looks good modulo last comments from Coleen posted on 11/24.<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<blockquote cite="mid:547c58b1-ce2c-4d5e-b823-11e01ef24a3a@default"
type="cite">
<div class="WordSection1">
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US">Coleen,<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US">I have tried to put in a module/class/function
summary comment in klassFactory.hpp – thanks.<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US">Updated webrev:<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/unified/unified_review/webrev04/" \
>http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/webrev04/</a><o:p></o:p></span></p>
>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US">Thanks again for your help<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US">Markus<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"
lang="EN-US"><o:p> </o:p></span></p>
<div>
<div style="border:none;border-top:solid #B5C4DF
1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span
style="font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext"
lang="EN-US">From:</span></b><span
style="font-size:10.0pt;font-family:"Tahoma","sans-serif";color:windowtext"
lang="EN-US"> Serguei Spitsyn <br>
<b>Sent:</b> den 24 november 2015 13:42<br>
<b>To:</b> Markus Gronlund<br>
<b>Cc:</b> serviceability-dev; Coleen Phillimore;
<a class="moz-txt-link-abbreviated" \
href="mailto:hotspot-runtime-dev@openjdk.java.net">hotspot-runtime-dev@openjdk.java.net</a><br>
<b>Subject:</b> Re: RFR(L): 8140485: Class load and
creation clean up<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal"> Markus,<br>
<br>
Sorry, this review is pretty late.<br>
<br>
I've passed through the rest of the file
classFileParcer.cpp.<br>
It is a thumbs up from me modulo minor comments in the first
email.<br>
<br>
In general, it is a great shift in the right direction (I
mean the refactoring).<br>
Not sure though, it is possible to use const modifiers
consistently, especially the after type const modifiers. :)<br>
<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
On 11/24/15 01:36, <a moz-do-not-send="true"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<p class="MsoNormal">Markus,<br>
<br>
<br>
I have reviewed almost all the code.<br>
It looks good so far.<br>
<br>
However, I've got lost at the end of the file
classFileParser.cpp<br>
that includes the functions <span \
class="changed">ClassFileParser::fill_instance_klass(),</span><br>
<span class="changed">ClassFileParser::</span>ClassFileParser(),
etc. <o:p></o:p></p>
<p class="MsoNormal"><br>
Apparently, it is very hard to make sure everything is
right as there are<br>
many changes there including newly written code but the
mapping<br>
between old and new code is lost in the all diff formats
including frames.<br>
<br>
Do you want me to continue to review this part, or you
have already<br>
good review coverage for this file? Do I have any extra
time for this?<br>
<br>
One issue I have is that I'm not sure if the staged
webrevs from the 1-st review round can<br>
still be used to help with this or they became obsolete in
the 2-nd and 3-rd round changes.<br>
<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
On 11/23/15 04:48, <a moz-do-not-send="true"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt">Hi
Markus,<br>
<br>
<br>
These are the comments I have at the moment. <br>
<br>
<br>
webrev03/src/share/vm/oops/constantPool.cpp<o:p></o:p></p>
<pre><span class="changed"> 67 static bool \
tag_array_is_zero_initialized(Array<u1>* tags) \
{</span><o:p></o:p></pre>
<pre><span class="changed"> 68 assert(tags != NULL, \
"invariant");</span><o:p></o:p></pre>
<pre><span class="changed"> 69 const int length = \
tags->length();</span><o:p></o:p></pre>
<pre> 70 for (int index = 0; index < length; index++) \
{<o:p></o:p></pre>
<pre><span class="changed"> 71 if (JVM_CONSTANT_Invalid != \
tags->at(index)) {</span><o:p></o:p></pre>
<pre><span class="changed"> 72 return \
false;</span><o:p></o:p></pre> <pre> 73 }<o:p></o:p></pre>
<pre><span class="new"> 74 }</span><o:p></o:p></pre>
<pre><span class="new"> 75 return true;</span><o:p></o:p></pre>
<pre><span class="new"> 76 }</span><o:p></o:p></pre>
<pre><span class="new"> 77 </span><o:p></o:p></pre>
<pre><span class="new"> 78 #endif</span><o:p></o:p></pre>
<pre><span class="new"> 79 </span><o:p></o:p></pre>
<pre><span class="new"> 80 ConstantPool::ConstantPool(Array<u1>* \
tags) :</span><o:p></o:p></pre>
<pre><span class="new"> 81 \
_length(tags->length()),</span><o:p></o:p></pre>
<pre><span class="new"> 82 _flags(0) {</span><o:p></o:p></pre>
<pre><span class="new"> 83 assert(tags != NULL, \
"invariant");</span><o:p></o:p></pre>
<pre><span class="new"> 84 assert(tags->length() == _length, \
"invariant");</span><o:p></o:p></pre>
<pre><span class="new"> 85 \
assert(tag_array_is_zero_initialized(tags), "invariant");</span><o:p></o:p></pre> \
<pre> 86 set_tags(tags);<o:p></o:p></pre> <pre><span class="new"> 87 \
</span><o:p></o:p></pre>
<pre><span class="new"> 88 // only set to non-zero if constant \
pool is merged by RedefineClasses</span><o:p></o:p></pre>
<pre><span class="new"> 89 set_version(0);</span><o:p></o:p></pre>
<pre><span class="new"> 90 </span><o:p></o:p></pre>
<pre><span class="new"> 91 assert(NULL == _cache, \
"invariant");</span><o:p></o:p></pre>
<pre><span class="new"> 92 assert(NULL == _reference_map, \
"invariant");</span><o:p></o:p></pre>
<pre><span class="new"> 93 assert(NULL == _resolved_references, \
"invariant");</span><o:p></o:p></pre>
<pre><span class="new"> 94 assert(NULL == _operands, \
"invariant");</span><o:p></o:p></pre>
<pre><span class="new"> 95 assert(NULL == _pool_holder, \
"invariant");</span><o:p></o:p></pre>
<pre><span class="new"> 96 // maybe its enough to just check one \
of the above</span><o:p></o:p></pre>
<pre><span class="new"> 97 // fields to verify underlying calloc \
allocation</span><o:p></o:p></pre> <pre> 98 }<o:p></o:p></pre>
<p class="MsoNormal" style="margin-bottom:12.0pt"><br>
<br>
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?<br>
I'd also suggest to follow the comment at 96-97: to
leave just one of the asserts 91-95 and remove the
comment.<br>
<br>
<br>
<br>
webrev03/src/share/vm/oops/arrayKlass.hpp<o:p></o:p></p>
<pre><span class="new"> 93 // Casting from \
Klass*</span><o:p></o:p></pre>
<pre><span class="new"> 94 static const ArrayKlass* cast(const \
Klass* k) {</span><o:p></o:p></pre>
<pre> 95 assert(k->is_array_klass(), "cast to \
ArrayKlass");<o:p></o:p></pre>
<pre><span class="changed"> 96 return static_cast<const \
ArrayKlass*>(k);</span><o:p></o:p></pre> <pre> 97 }<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre> Minor: The comment at line 93 is not fully correct as the \
casting is from 'const Klass*'.<o:p></o:p></pre>
<pre> The same issue is in some other updated \
files:<o:p></o:p></pre>
<pre> instanceKlass.hpp, objArrayKlass.hpp, \
typeArrayKlass.hpp<o:p></o:p></pre> <pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre><o:p> </o:p></pre>
<pre>webrev03/src/share/vm/runtime/vmStructs.cpp<o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre> Minor: Line 323: Unaligned '\' at the end.<o:p></o:p></pre>
<p class="MsoNormal"><br>
<br>
<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
On 11/20/15 11:02, Coleen Phillimore wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><br>
Hi Markus, <br>
<br>
I ran your n-1 change through my local tests and they
all passed. <br>
<br>
The only thing left is I think adding comments would be
good to KlassFactory.hpp/cpp. <br>
<br>
Thanks, <br>
Coleen <br>
<br>
On 11/20/15 1:27 PM, Markus Gronlund wrote: <br>
<br>
<o:p></o:p></p>
<p class="MsoNormal">Hi Coleen, <br>
<br>
Many, many thanks for taking on this large change. <br>
<br>
Thanks for the feedback, I have updated like this: <br>
<br>
ClassFactory::create(...) ->
KlassFactory::create_from_stream(...) <br>
<br>
// "Klass" better signifies internal representation
(metadata) compared to "Class" which has a more
classfile contextual association. Fixed. <br>
// In addition, I have removed all overloaded
create_from_stream() functions, reducing them to a
single create_from_stream(). The caller will <br>
// have to pass NULL on unused parameters with this
tradeoff. <br>
<br>
// renaming <br>
ClassFileParser::instance_klass(...) ->
ClassFileParser::create_instance_klass(...) <br>
<br>
ClassFileParser::fill_instance(...) ->
ClassFileParser::fill_instance_klass(...) <br>
<br>
// moved the parse_stream() and post_process_stream()
into the ClassFileParser constructor (process() removed)
- thanks! <br>
<br>
Here is an updated webrev: <br>
<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/unified/unified_review/webrev03/" \
>http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/webrev03/</a> \
> <br>
<br>
Many thanks again <br>
Markus <br>
<br>
<br>
-----Original Message----- <br>
From: Coleen Phillimore <br>
Sent: den 20 november 2015 04:51 <br>
To: <a moz-do-not-send="true"
href="mailto:hotspot-runtime-dev@openjdk.java.net">hotspot-runtime-dev@openjdk.java.net</a>
<br>
Subject: Re: RFR(L): 8140485: Class load and creation
clean up <br>
<br>
<br>
Hi, More comments on classFileParser. <br>
<br>
It seems like the ClassFileParser::process is
unnecessary as a <br>
function. I think the 3 lines should be imported into
<br>
ClassFileParser::ClassFileParser and it makes sense
there. <br>
<br>
<br>
<o:p></o:p></p>
<p class="MsoNormal">Summary: <br>
<br>
Reduce the visibility of CFP impl methods by internal
linkage instead of external linkage. <br>
<br>
Comment: <br>
<br>
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". <o:p></o:p></p>
<p class="MsoNormal">Yes, I agree and appreciate moving
all of these functions static and internal in CFP. <br>
<br>
Coleen <br>
<br>
<br>
On 11/12/15 7:08 AM, Markus Gronlund wrote: <br>
<br>
<o:p></o:p></p>
<p class="MsoNormal">Hi again, <br>
<br>
I have reworked and simplified this clean up/refactoring
suggestion: <br>
<br>
Updated high-level description: <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/unified/unified_review/81">http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/81</a>
<br>
40485_updated.jpg <br>
<br>
Updated webrev: <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/unified/unified_review/we">http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/we</a>
<br>
brev02/ <br>
<br>
Updated patch: <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/unified/unified_review/81">http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/81</a>
<br>
40485_open_unified_updated.patch <br>
<br>
Thanks in advance <br>
Markus <br>
<br>
<br>
-----Original Message----- <br>
From: Markus Gronlund <br>
Sent: den 27 oktober 2015 13:21 <br>
To: <a moz-do-not-send="true"
href="mailto:hotspot-runtime-dev@openjdk.java.net">hotspot-runtime-dev@openjdk.java.net</a>;
<br>
<a moz-do-not-send="true"
href="mailto:serviceability-dev@openjdk.net">serviceability-dev@openjdk.net</a>
<br>
Subject: RFR(L): 8140485: Class load and creation clean
up <br>
<br>
Greetings, <br>
<br>
<br>
I have spent some time refactoring and cleaning up parts
of the class load and creation sequence and code. <br>
<br>
<br>
Bug: <a moz-do-not-send="true"
href="https://bugs.openjdk.java.net/browse/JDK-8140485">https://bugs.openjdk.java.net/browse/JDK-8140485</a>
<br>
<br>
Overview: <br>
<br>
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). <br>
<br>
<br>
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). <br>
<br>
<br>
Use case: <br>
<br>
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. <br>
<br>
<br>
Cleanup / refactoring objectives: <br>
<br>
<br>
- Allow for class parsing implementation reuse
<br>
<br>
- Decouple class parsing and Klass creation <br>
<br>
- Decouple JVMTI class file load hook from
ClassFileParser <br>
<br>
- Add compiler assisted read-only intent in
order to assist future refactoring attempts <br>
<br>
- "componentify" ClassFileParser / apply to
Klass/IK/AK creations <br>
<br>
- Take advantage of some optimizations
opportunities (constructors and underlying "calloc"
memory) <br>
<br>
- Maintain class load performance <br>
<br>
<br>
High level diagram explaining the updated suggested
sequence (high level): <br>
<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/8140485_Class_load_and_cr">http://cr.openjdk.java.net/~mgronlun/8140485/8140485_Class_load_and_cr</a>
<br>
eation_cleanup_high_level_sequence.jpg <br>
<br>
<br>
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): <br>
<br>
<br>
Sequence: "first" (order of changes) <br>
<br>
Name: "8140485_1_unified entry point" <br>
<br>
Link: <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/split/first/webrev01/">http://cr.openjdk.java.net/~mgronlun/8140485/split/first/webrev01/</a>
<br>
<br>
Summary: <br>
<br>
Split out JVMTI agent CFLH from CFP - moved to
SD::create_vm_representation_prologue(). <br>
<br>
Channel all class loading via a unified entry point - <br>
SD::create_vm_representation() <br>
<br>
Split out ClassPathEntry into own header. <br>
<br>
Creation of new class ClassPathFileStream (specialized
ClassPathStream) to retain ClassLoaderExt::Context
functionality in the refactored code. <br>
<br>
"Need verify" property is carried within the CFS itself.
<br>
<br>
Comment: <br>
<br>
SystemDictionary::load_instance_class() will now have a
call to <br>
"load_class_from_classpath" - the relevance of this name
might need to <br>
be modified (modules etc?) <br>
<br>
<br>
Sequence: "second" <br>
<br>
Name: "8140485_2_read_only_class_file_stream" <br>
<br>
Link: <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/split/second/webrev01/">http://cr.openjdk.java.net/~mgronlun/8140485/split/second/webrev01/</a>
<br>
<br>
Summary: <br>
<br>
The underlying bytestream (buffer) in a CFS
representation should not be altered (advertently or
inadvertently) once created, made read-only. <br>
<br>
Comment: <br>
<br>
Modifications should work with copies. <br>
<br>
<br>
Sequence: "third" <br>
<br>
Name: "8140485_3_componentify_class_file_parser" <br>
<br>
Link: <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/split/third/webrev01/">http://cr.openjdk.java.net/~mgronlun/8140485/split/third/webrev01/</a>
<br>
<br>
Summary: <br>
<br>
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). <br>
<br>
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.. <br>
<br>
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). <br>
<br>
<br>
"Consts everywhere.." <br>
<br>
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. <br>
<br>
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. <br>
<br>
<br>
Accessors - CFP becomes a data container to be used in
constructors <br>
(see change seven). In addition, you will now have a
possibility to <br>
fetch data from a parser without creating a fullblown
Klass <br>
<br>
<br>
Comment: <br>
<br>
I think the code could be improved and more refactoring
could be <br>
accomplished by introducing a Metaspace allocation RAII
class - this <br>
would allow the functions to decouple even more that is
currently <br>
(might be next steps) <br>
<br>
<br>
Sequence: "fourth" <br>
<br>
Name: "8140485_4_hide_impl_details_class_file_parser" <br>
<br>
Link: <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/split/fourth/webrev01/">http://cr.openjdk.java.net/~mgronlun/8140485/split/fourth/webrev01/</a>
<br>
<br>
Summary: <br>
<br>
Reduce the visibility of CFP impl methods by internal
linkage instead of external linkage. <br>
<br>
Comment: <br>
<br>
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". <br>
<br>
(that is, it can use an external interface alt. might
not need access <br>
at all (helpers)) <br>
<br>
<br>
Sequence: "fifth" <br>
<br>
Name: "8140485_5_update_entry_point" <br>
<br>
Link: <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/split/fifth/webrev01/">http://cr.openjdk.java.net/~mgronlun/8140485/split/fifth/webrev01/</a>
<br>
<br>
Summary: <br>
<br>
Update signatures. Remove the parameter passing where
not needed <br>
("parsed_name" reference for example) <br>
<br>
<br>
Sequence: "sixth" <br>
<br>
Name: "8140485_6_types_const_includes_repercussions" <br>
<br>
Link: <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/split/sixth/webrev01/">http://cr.openjdk.java.net/~mgronlun/8140485/split/sixth/webrev01/</a>
<br>
<br>
Summary: <br>
<br>
Rippling effects of stronger constness. <br>
<br>
Forward includes where can be applied. This revealed
other code modules not having includes -added. <br>
<br>
Downcasting of const qualified types (applied Scott
Meyers idiom of <br>
dual const_cast additions (see Effective C++)) <br>
<br>
<br>
Sequence: "seventh" <br>
<br>
Name:
"8140485_7_applied_use_in_constructors_and_some_optimizations"
<br>
<br>
Link: <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/split/seventh/webrev01/">http://cr.openjdk.java.net/~mgronlun/8140485/split/seventh/webrev01/</a>
<br>
<br>
Summary: <br>
<br>
Take advantage of above modifications to gain direct
upshots in other areas. Use CFP as the data container
when constructing Klass/IK/AK. <br>
<br>
Optimizations: Reduce/optimize
Klass/InstanceKlass/ArrayKlass/ConstantPool constructors
by using underlying invariant on "calloc" equivalent
MetaspaceObj allocations. <br>
<br>
<br>
Unified change/patch: <br>
<br>
Webrev: <a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/unified/webrev01/">http://cr.openjdk.java.net/~mgronlun/8140485/unified/webrev01/</a>
<br>
<br>
Patch: <br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Emgronlun/8140485/unified/8140485_unified.p">http://cr.openjdk.java.net/~mgronlun/8140485/unified/8140485_unified.p</a>
<br>
atch <br>
<br>
Summary: <br>
<br>
Unified (folded) patch for easier application/imports,
for <br>
compilation/try out <br>
<br>
<br>
Testing: <br>
<br>
I have done targeted testing during development
especially using the <br>
"runtime/ParallelClassLoading" test suite in order to
ensure <br>
functional equivalency. JPRT testing. Performance tests
(see below) <br>
<br>
<br>
Performance: <br>
<br>
Unstructured: <br>
<br>
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. <br>
<br>
<br>
Structured: <br>
<br>
Startup/footprint-benchmarks (Linux32, Linux64,
Windows32, Windows64, <br>
Solaris SPARC-T) <br>
<br>
<br>
It is hard to read anything decisive around these (the
effects most <br>
likely hides in noise), but at least determine that the
changes do not <br>
introduce anything statistically significant (+/-) (both
time and <br>
space) <br>
<br>
Thanks in advance <br>
<br>
Markus <br>
<br>
<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</blockquote>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic