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-...@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.spit...@oracle.com
<mailto:serguei.spit...@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.spit...@oracle.com
<mailto:serguei.spit...@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-...@openjdk.java.net
<mailto:hotspot-runtime-...@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-...@openjdk.java.net
<mailto:hotspot-runtime-...@openjdk.java.net>;
serviceability-...@openjdk.net
<mailto:serviceability-...@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
