Re: RFR (M) Close alignment gaps in InstanceKlass
Hi Coleen. The JVMCI changes look OK. It looks like there is a Graal unittest that covers getSourceFileName, but those tests don't always get run. If it's not too much trouble, could you look into enabling getSourceFileName() testing in test/hotspot/jtreg/compiler/jvmci/jdk.vm.ci.runtime.test/src/jdk/vm/ci/runtime/test/TestResolvedJavaType.java ? It's currently on the "untested" list. thanks, dl On 4/21/20 1:12 PM, coleen.phillim...@oracle.com wrote: Summary: moved fields around and some constant fields into ConstantPool This is a simple change except that I moved some constant fields from InstanceKlass into the constant pool so they can be shared read-only in the CDS archive. There are associated repercussions in SA and JVMCI, so please look at these changes. Also moved similarly sized fields together in the class so there's less likelihood of introducing gaps in future InstanceKlass changes. InstanceKlass is reduced from 544 to 520 bytes in a simple Hello World class. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8238048 Tested with tier1-6. Thanks, Coleen
Re: RFR(XS) 8243210: ClhsdbScanOops fails with NullPointerException in FileMapHeader.inCopiedVtableSpace
Hi Chris, It looks reasonable. Thanks, Serguei On 4/21/20 16:07, Chris Plummer wrote: Hello, Please review the following: https://bugs.openjdk.java.net/browse/JDK-8243210 http://cr.openjdk.java.net/~cjplummer/8243210/webrev.01/index.html Description is in the bug. Hopefully this is the last in a series of about 6 bugs being addressed that finally lets us get this test off the problem list for all platforms. thanks, Chris
Re: RFR(L): 8215624: add parallel heap inspection support for jmap histo(G1)
Dear all, May I ask you help to review? This RFR has been there for quite a while. Thanks! BRs, Lin > On 2020/3/16, 5:18 PM, "linzang(臧琳)" wrote:> > Just update a new path, my preliminary measure show about 3.5x speedup of > jmap histo on a nearly full 4GB G1 heap (8-core platform with parallel thread > number set to 4). >webrev: http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_02/ >bug: https://bugs.openjdk.java.net/browse/JDK-8215624 >CSR: https://bugs.openjdk.java.net/browse/JDK-8239290 >BRs, > Lin > > On 2020/3/2, 9:56 PM, "linzang(臧琳)" wrote: > > > >Dear all, > > Let me try to ease the reviewing work by some explanation :P > > The patch's target is to speed up jmap -histo for heap > iteration, from my experience it is necessary for large heap investigation. > E.g in bigData scenario I have tried to conduct jmap -histo against 180GB > heap, it does take quite a while. > > And if my understanding is corrent, even the jmap -histo > without "live" option does heap inspection with heap lock acquired. so it is > very likely to block mutator thread in allocation-sensitive scenario. I would > say the faster the heap inspection does, the shorter the mutator be blocked. > This is parallel iteration for jmap is necessary. > > I think the parallel heap inspection should be applied to all > kind of heap. However, consider the heap layout are different for GCs, much > time is required to understand all kinds of the heap layout to make the whole > change. IMO, It is not wise to have a huge patch for the whole solution at > once, and it is even harder to review it. So I plan to implement it > incrementally, the first patch (this one) is going to confirm the > implemention detail of how jmap accept the new option, passes it to > attachListener of the jvm process and then how to make the parallel > inspection closure be generic enough to make it easy to extend to different > heap layout. And also how to implement the heap inspection in specific gc's > heap. This patch use G1's heap as the begining. > > This patch actually do several things: > > 1. Add an option "parallelThreadNum=" to jmap -histo, the > default behavior is to set N to 0, means let's JVM decide how many threads to > use for heap inspection. Set this option to 1 will disable parallel heap > inspection. (more details in CSR: > https://bugs.openjdk.java.net/browse/JDK-8239290) > > 2. Make a change in how Jmap passing arguments, changes in > http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_01/src/jdk.jcmd/share/classes/sun/tools/jmap/JMap.java.udiff.html, > originally it pass options as separate arguments to attachListener, this > patch change to that all options be compose to a single string. So the > arg_count_max in attachListener.hpp do not need to be changed, and hence > avoid the compatibility issue, as disscussed at > https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-March/027334.html > > 3. Add an abstract class ParHeapInspectTask in > heapInspection.hpp / heapInspection.cpp, It's work(uint worker_id) method > prepares the data structure (KlassInfoTable) need for every parallel worker > thread, and then call do_object_iterate_parallel() which is heap specific > implementation. I also added some machenism in KlassInfoTable to support > parallel iteration, such as merge(). > >4. In specific heap (G1 in this patch), create a subclass of > ParHeapInspectTask, implement the do_object_iterate_parallel() for parallel > heap inspection. For G1, it simply invoke g1CollectedHeap's > object_iterate_parallel(). > >5. Add related test. > >6. it may be easy to extend this patch for other kinds of heap > by creating subclass of ParHeapInspectTask and implement the > do_object_iterate_parallel(). > > > >Hope these info could help on code review and initate the > discussion :-) > >Thanks! > > > >BRs, > >Lin > >>On 2020/2/19, 9:40 AM, "linzang(臧琳)" wrote:. > >> > >> Re-post this RFR with correct enhancement number to make it > trackable. > >> please ignore the previous wrong post. sorry for troubles. > >> > >> webrev: > http://cr.openjdk.java.net/~lzang/jmap-8214535/8215624/webrev_01/ > >>Hi bug: https://bugs.openjdk.java.net/browse/JDK-8215624 > >>CSR: https://bugs.openjdk.java.net/browse/JDK-8239290 > >>-- > >>Lin > >>>Hi Lin, > >>> > >>>Could you, please, re-post your RFR with the right > enhancement number in > >>>the message subject? > >>>It will be more trackable this way. > >>> > >>>Thanks, >
RFR(XS) 8243210: ClhsdbScanOops fails with NullPointerException in FileMapHeader.inCopiedVtableSpace
Hello, Please review the following: https://bugs.openjdk.java.net/browse/JDK-8243210 http://cr.openjdk.java.net/~cjplummer/8243210/webrev.01/index.html Description is in the bug. Hopefully this is the last in a series of about 6 bugs being addressed that finally lets us get this test off the problem list for all platforms. thanks, Chris
Re: RFR (M) Close alignment gaps in InstanceKlass
On 4/21/20 4:41 PM, Chris Plummer wrote:
Hi Coleen,
The SA changes look good. BTW, I've already made the
TestIntConstant.java change in the Loom project.
InstanceKlass::_misc_is_unsafe_anonymous was already changed to "8" in
loom. You might want to sync up with Ron to make sure you aren't
making conflicting changes in this area. Your code looks like:
249 enum {
250 _misc_rewritten = 1 << 0, //
methods rewritten.
251 _misc_has_nonstatic_fields = 1 << 1, // for
sizing with UseCompressedOops
252 _misc_should_verify_class = 1 << 2, // allow
caching of preverification
253 _misc_is_unsafe_anonymous = 1 << 3, // has
embedded _unsafe_anonymous_host field
254 _misc_is_contended = 1 << 4, //
marked with contended annotation
Loom looks like:
static const unsigned _misc_kind_field_size = 0;
...
// Start after _misc_kind field.
enum {
_misc_rewritten = 1 <<
(_misc_kind_field_size + 0), // methods rewritten.
_misc_has_nonstatic_fields = 1 <<
(_misc_kind_field_size + 1), // for sizing with UseCompressedOops
_misc_should_verify_class = 1 <<
(_misc_kind_field_size + 2), // allow caching of preverification
_misc_is_unsafe_anonymous = 1 <<
(_misc_kind_field_size + 3), // has embedded _unsafe_anonymous_host
field
_misc_is_contended = 1 <<
(_misc_kind_field_size + 4), // marked with contended annotation
...
At the very least this is a merge conflict that loom will need to deal
with and it would help to know about in advance (Alan normally does
the merges).
Actually I just had a look at the loom instanceKlass.hpp and this change
helps that. Loom adds a kind of InstanceKlass, so changed the field to a
u1 as I have done. This will clean up loom too.
Thanks,
Coleen
thanks,
Chris
On 4/21/20 1:12 PM, coleen.phillim...@oracle.com wrote:
Summary: moved fields around and some constant fields into ConstantPool
This is a simple change except that I moved some constant fields from
InstanceKlass into the constant pool so they can be shared read-only
in the CDS archive. There are associated repercussions in SA and
JVMCI, so please look at these changes. Also moved similarly sized
fields together in the class so there's less likelihood of
introducing gaps in future InstanceKlass changes.
InstanceKlass is reduced from 544 to 520 bytes in a simple Hello
World class.
open webrev at
http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8238048
Tested with tier1-6.
Thanks,
Coleen
Re: RFR (M) Close alignment gaps in InstanceKlass
On 4/21/20 4:41 PM, Chris Plummer wrote:
Hi Coleen,
The SA changes look good. BTW, I've already made the
TestIntConstant.java change in the Loom project.
InstanceKlass::_misc_is_unsafe_anonymous was already changed to "8" in
loom. You might want to sync up with Ron to make sure you aren't
making conflicting changes in this area. Your code looks like:
249 enum {
250 _misc_rewritten = 1 << 0, //
methods rewritten.
251 _misc_has_nonstatic_fields = 1 << 1, // for
sizing with UseCompressedOops
252 _misc_should_verify_class = 1 << 2, // allow
caching of preverification
253 _misc_is_unsafe_anonymous = 1 << 3, // has
embedded _unsafe_anonymous_host field
254 _misc_is_contended = 1 << 4, //
marked with contended annotation
Loom looks like:
static const unsigned _misc_kind_field_size = 0;
...
// Start after _misc_kind field.
enum {
_misc_rewritten = 1 <<
(_misc_kind_field_size + 0), // methods rewritten.
_misc_has_nonstatic_fields = 1 <<
(_misc_kind_field_size + 1), // for sizing with UseCompressedOops
_misc_should_verify_class = 1 <<
(_misc_kind_field_size + 2), // allow caching of preverification
_misc_is_unsafe_anonymous = 1 <<
(_misc_kind_field_size + 3), // has embedded _unsafe_anonymous_host
field
_misc_is_contended = 1 <<
(_misc_kind_field_size + 4), // marked with contended annotation
...
At the very least this is a merge conflict that loom will need to deal
with and it would help to know about in advance (Alan normally does
the merges).
It looks like there are other changes in loom that motivated changing it
like this. Why is misc_kind_field_size = 0?
Did loom add more misc_flags ? It turns out that there are plenty of
available flags in the access_flags if more are needed.
Thank you for looking at the SA changes.
Coelen
thanks,
Chris
On 4/21/20 1:12 PM, coleen.phillim...@oracle.com wrote:
Summary: moved fields around and some constant fields into ConstantPool
This is a simple change except that I moved some constant fields from
InstanceKlass into the constant pool so they can be shared read-only
in the CDS archive. There are associated repercussions in SA and
JVMCI, so please look at these changes. Also moved similarly sized
fields together in the class so there's less likelihood of
introducing gaps in future InstanceKlass changes.
InstanceKlass is reduced from 544 to 520 bytes in a simple Hello
World class.
open webrev at
http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8238048
Tested with tier1-6.
Thanks,
Coleen
Re: RFR (M) Close alignment gaps in InstanceKlass
Hi Coleen,
The SA changes look good. BTW, I've already made the
TestIntConstant.java change in the Loom project.
InstanceKlass::_misc_is_unsafe_anonymous was already changed to "8" in
loom. You might want to sync up with Ron to make sure you aren't making
conflicting changes in this area. Your code looks like:
249 enum {
250 _misc_rewritten = 1 << 0, //
methods rewritten.
251 _misc_has_nonstatic_fields = 1 << 1, // for
sizing with UseCompressedOops
252 _misc_should_verify_class = 1 << 2, // allow
caching of preverification
253 _misc_is_unsafe_anonymous = 1 << 3, // has
embedded _unsafe_anonymous_host field
254 _misc_is_contended = 1 << 4, // marked
with contended annotation
Loom looks like:
static const unsigned _misc_kind_field_size = 0;
...
// Start after _misc_kind field.
enum {
_misc_rewritten = 1 <<
(_misc_kind_field_size + 0), // methods rewritten.
_misc_has_nonstatic_fields = 1 <<
(_misc_kind_field_size + 1), // for sizing with UseCompressedOops
_misc_should_verify_class = 1 <<
(_misc_kind_field_size + 2), // allow caching of preverification
_misc_is_unsafe_anonymous = 1 <<
(_misc_kind_field_size + 3), // has embedded _unsafe_anonymous_host field
_misc_is_contended = 1 <<
(_misc_kind_field_size + 4), // marked with contended annotation
...
At the very least this is a merge conflict that loom will need to deal
with and it would help to know about in advance (Alan normally does the
merges).
thanks,
Chris
On 4/21/20 1:12 PM, coleen.phillim...@oracle.com wrote:
Summary: moved fields around and some constant fields into ConstantPool
This is a simple change except that I moved some constant fields from
InstanceKlass into the constant pool so they can be shared read-only
in the CDS archive. There are associated repercussions in SA and
JVMCI, so please look at these changes. Also moved similarly sized
fields together in the class so there's less likelihood of introducing
gaps in future InstanceKlass changes.
InstanceKlass is reduced from 544 to 520 bytes in a simple Hello World
class.
open webrev at http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8238048
Tested with tier1-6.
Thanks,
Coleen
RFR (M) Close alignment gaps in InstanceKlass
Summary: moved fields around and some constant fields into ConstantPool This is a simple change except that I moved some constant fields from InstanceKlass into the constant pool so they can be shared read-only in the CDS archive. There are associated repercussions in SA and JVMCI, so please look at these changes. Also moved similarly sized fields together in the class so there's less likelihood of introducing gaps in future InstanceKlass changes. InstanceKlass is reduced from 544 to 520 bytes in a simple Hello World class. open webrev at http://cr.openjdk.java.net/~coleenp/2020/8238048.01/webrev bug link https://bugs.openjdk.java.net/browse/JDK-8238048 Tested with tier1-6. Thanks, Coleen
RFR (S) 8243244: CSR Add option to jcmd to write a gzipped heap dump
Hi, I need a review for the following CSR: https://bugs.openjdk.java.net/browse/JDK-8243244 It describes the two additional options added to the GC.heap_dump command to support gzipped heap dumps. Best regards, Ralf
Re: RFR: JDK-8242943 Fix all remaining unchecked warnings in jdk.hotspot.agent
On 2020-04-20 21:19, Joe Darcy wrote: Hello, On 4/16/2020 5:47 AM, Magnus Ihse Bursie wrote: [snip] The tricky one here is the helper TableModelComparator. This one took me a while to wrap my head around. It implements Comparator, but the compare() method takes "rows" from the model, which are just expressed as Objects, and left to subclasses to define differently. For one of the subclasses it uses the type T that the TableModel is created around, but in other it uses an independent domain model. :-/ Anyway. The compare() method then extracts data for the individual columns of each row using the method getValueForColumn(), and compare them pairwise. This data from the rows are supposed to implement Comparable. In the end, I think I got it pretty OK, but I'm still an apprentice when it comes to generics black magic. The subclasses of TableModelComparator want to return different objects from getValueForColumn() for different columns in the row, like Long or String. They are all Comparable, but String is Comparable and Long is Comparable. So I needed to specify the abstract method as returning Comparable, since Comparable is not Comparable. But then, for reasons that I do not fully fathom, I could not specify Comparable o1 = getValueForColumn(row1, columns[i]); Comparable o2 = getValueForColumn(row2, columns[i]); int result = o1.compareTo(o2); because the compiler did not accept the call to compareTo(). I did try to sacrifice a black rooster at midnight and walking backwards in a circle three time, to no avail. Maybe the problem was that it was not full moon or that I have no cat. In the end, I casted o1 and o2 to Comparable and suppressed the warning for that cast. If anyone knows what rituals I need to perform to make this work, or -- even better -- how to fix the code properly, please let me know! A brief discussion of wildcards, ?. The meaning of a wildcard is some particular unknown type, meeting the various constraints from "? extends" or "? super". If there is no explicit constraint listed, it is equivalent to "? extends Object". So the meaning of the code above is something Comparable o1 = getValueForColumn(row1, columns[i]); Comparable o2 = getValueForColumn(row2, columns[i]); where S and T are two fresh, unrelated type variables. Concretely, this means S and T could be, say, String and Integer so that is why a call to o1.compareTo(o2) would not be accepted by the compiler since Strings and Integer aren't comparable to each other. Ah, right, and compareTo -- in contrast with equals() -- only compares to objects of the same type. I think that's part of what confused me here. While two instances of "Comparable" are syntactically the same, they don't represent the same type inside the compiler. In some cases, you can get around this issue by using a separate method to capture the type variable and then use it more than once. A technique we've used in the JDK is to have a pubic method with a wildcards calling a private method using a type variable. I've looked around for a few minutes and didn't find a concrete example in the JDK, but they're in there. I haven't looked over the specific code here in much detail; the type Comparable might to be the best you can do, but it isn't very expressive since Object's aren't necessarily comparable. Thank you for taking time to explain this! /Magnus HTH, -Joe
RE: RFR(L) 8237354: Add option to jcmd to write a gzipped heap dump
Hi Ralf,
I've just started a closer review of your change, based on your current webrev.
I'm not done yet but hope I'll find the time to finish this in the next few
days.
However, as this patch seems in a quite good condition already and we're
targeting JDK15, could you please start creating the required CSR to get it
through the CSR process in parallel to the final review?
Thanks
Christoph
> -Original Message-
> From: Schmelter, Ralf
> Sent: Montag, 20. April 2020 10:14
> To: Reingruber, Richard ; Ioi Lam
> ; Langer, Christoph ;
> Yasumasa Suenaga ;
> serguei.spit...@oracle.com; hotspot-runtime-...@openjdk.java.net
> runtime
> Cc: serviceability-dev@openjdk.java.net
> Subject: RE: RFR(L) 8237354: Add option to jcmd to write a gzipped heap
> dump
>
> Hi Richard,
>
> thanks for the review. I have incorporated your remarks into a new webrev:
> http://cr.openjdk.java.net/~rschmelter/webrevs/8237354/webrev.2/
>
> Some remarks to specific points:
>
> > ### src/hotspot/share/services/heapDumper.cpp
> > 762: assert(_active, "Must be active");
> >
> > It appears to me that the assertion would fail, if an error occurred
> > creating
> the CompressionBackend.
>
> You are supposed to check for errors after creating the DumpWriter (which
> creates the CompressionBackend). And in case of an error, you directly
> destruct the object. I've added a comment to make that clear.
>
> > 767: I think _current->in_used doesn't take the final 9 bytes into account
> that are written in
> > DumperSupport::end_of_dump() after the last dump segment has been
> finished.
> > You could call get_new_buffer() instead of the if clause.
>
> Wow, how did you found this? I've fixed it by making sure we flush the
> DumpWriter before calling the deactivate method.
>
> > 1064: DumpWriter::DumpWriter()
> >
> > There doesn't seem to be enough error handling if _buffer cannot be
> allocated.
> > E.g. DumpWriter::write_raw() at line 1091 will enter an endless loop.
>
> As described above, this will not happen if we check for error after
> constructing the DumpWriter.
>
> > ### src/java.base/share/native/libzip/zip_util.c
> > 1610: Will be hard to beat zlib_block_alloc() and zlib_block_free()
> performance wise. But have you
> > measured the performance gain? In other words: is it worth it? :)
>
> This is not done for performance, but to make sure the allocation will not
> fail
> midway during writing the dump. Maybe it is not worth it, though.
>
> > 1655: The result of deflateBound() seems to depend on the header
> comment, which is not given
> > here. Could this be an issue, because ZIP_GZip_Fully() can take a
> comment?
>
> I've added a 1024 byte additional bytes to avoid the problem.
>
> > ### test/lib/jdk/test/lib/hprof/parser/Reader.java
> >
> > 93: is the created GzipRandomAccess instance closed somewhere?
>
> The object is not closed since it is still used by the Snapshot returned.
>
> Best regard,
> Ralf
>
>
> -Original Message-
> From: Reingruber, Richard
> Sent: Tuesday, 14 April 2020 10:30
> To: Schmelter, Ralf ; Ioi Lam
> ; Langer, Christoph ;
> Yasumasa Suenaga ;
> serguei.spit...@oracle.com; hotspot-runtime-...@openjdk.java.net
> runtime
> Cc: serviceability-dev@openjdk.java.net
> Subject: RE: RFR(L) 8237354: Add option to jcmd to write a gzipped heap
> dump
>
> Hi Ralf,
>
> thanks for providing this enhancement to parallel gzip-compress heap
> dumps!
>
> I reckon it's safe to say that the coding is sophisticated. It would be
> awesome
> if you could sketch
> the idea of how HeapDumper, DumpWriter and CompressionBackend work
> together to produce the gzipped
> dump in a source code comment. Just enough to get started if somebody
> should ever have to track down
> a bug -- an unlikely event, I know ;)
>
> Please find the details of my review below.
>
> Thanks, Richard.
> // Not Reviewer
>
> --
>
> ### src/hotspot/share/services/diagnosticCommand.cpp
>
> 510 _gzip_level("-gz-level", "The compression level from 0 (store) to 9
> (best) when writing in gzipped format.",
> 511 "INT", "FALSE", "1") {
>
> "FALSE" should be probably false.
>
> ### src/hotspot/share/services/diagnosticCommand.hpp
> Ok.
>
> ### src/hotspot/share/services/heapDumper.cpp
>
> 390: Typo: initized
>
> 415: Typo: GZipComressor
>
> 477: Could you please add a comment, how the "HPROF BLOCKSIZE"
> comment is helpful?
>
> 539: Member variables of WriteWork are missing the '_' prefix.
>
> 546: Just a comment: WriteWork::in_max is actually a compile time constant.
> Would be nice if it could be
> declared so. One could use templates for this, but then my favourite ide
> (eclipse cdt) doesn't
> show me references and call hierarchies anymore. So I don't think it is
> worth it.
>
> 591: Typo: Removes the first element. Returns NULL is empty.
>
> 663: _writer, _compressor, _lock could be const.
>
> 762: assert(_active, "Must be active");
>
> It appears to me that the asserti
