Re: RFR(L): 8140485: Class load and creation clean up

Coleen Phillimore Tue, 08 Dec 2015 08:45:20 -0800


On 12/8/15 5:35 AM, Markus Gronlund wrote:

Harold, Coleen and Serguei,

Thanks for your help in reviewing so far.

I have created an updated changeset which I intend to push if you are ok with 
it, please see:

http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/webrev05/


Coleen,

I went back to the CheckIntrinsic piece, and true, it was lost in the merging - 
thanks for pointing this out.
I have refactored that piece into a static function.

Very nice. Not your change but I wonder how efficient is it toadditionally iterate through the methods for each class file parsed tocheck for intrinsics.


In addition, I reworked the is_public() member function to become is_internal() 
instead.

I have removed the explicit usages of the class_loader oop - thanks again. 
Where needed this will be picked up from the ClassLoaderData.

I have attempted to pack the fields better for ClassFileParser - thanks.


This all looks great and a much needed cleanup to this code!

Coleen



Harold,

Thanks for your help in reviewing this!

About your comments:

1. In classLoader.cpp could you add comments explaining why no_verification is 
specified to the ClassFileStream constructor?

[MG] Although these streams are currently loaded by the primordial class loader (which is 
the reason I construct them passing no_verification) and a comment such as "// the 
primordial class loader loads classes from a trusted repository" would be useful - I 
have now reconsidered this:

I will now pass ClassFileStream::verify when constructing these streams as well 
- the verification setting will be recalculated (updated and set) after stream 
creation at least twice before reaching the verification code. For most of 
these streams, the recalculated verification setting done after construction 
will be no_verification. It might be prudent in light of security to construct 
the streams defaulting to verify. This is also the default parameter for the 
ClassFileStream - however, I am passing them to be explicit.

2. Could you replace "invariant" with a more interesting message where 
appropriate in some of the asserts?

[MG] I agree that this would probably be helpful, but after I started an 
attempt in doing this, I realized that it would take quite some time to come up 
with some descriptive messages - maybe it would be ok to take this after 
integration?

3.  In reflection.cpp, could you replace the switch statement in
get_mirror_from_signature() with an if/else statement?

[MG] Done!

Runtime/ParallelClassLoading tests completed.

JPRT checkin dry-run completed.

Thanks for your help

/Markus



-----Original Message-----
From: harold seigel
Sent: den 1 december 2015 22:49
To: hotspot-runtime-...@openjdk.java.net
Subject: Re: RFR(L): 8140485: Class load and creation clean up

Hi Markus,

The restructuring looks good!

I just have a few minor comments.

1. In classLoader.cpp could you add comments explaining why no_verification is 
specified to the ClassFileStream constructor?

2. Could you replace "invariant" with a more interesting message where 
appropriate in some of the asserts?

3.  In reflection.cpp, could you replace the switch statement in
get_mirror_from_signature() with an if/else statement?

Thanks! Harold

On 11/20/2015 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/we
brev03/

Many thanks again
Markus


-----Original Message-----
From: Coleen Phillimore
Sent: den 20 november 2015 04:51
To: 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/8
1
40485_updated.jpg

Updated webrev:
http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/w
e
brev02/

Updated patch:
http://cr.openjdk.java.net/~mgronlun/8140485/unified/unified_review/8
1
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;
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_c
r 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

Re: RFR(L): 8140485: Class load and creation clean up

Reply via email to