Re: RFR: 8308762: Metaspace leak with Instrumentation.retransform [v5]
> Fix a small leak in constant pool merging during retransformation of a class. > If this class has a catch block with `Throwable`, the class `Throwable` is > pre-resolved in the constant pool, while all the other classes are in a > unresolved state. So the constant pool merging process was considering the > entry with pre-resolved class as different compared to the destination and > create a new entry. We now try to consider it as equal specially for > Methodref/Fieldref. Jean-Philippe Bempel has updated the pull request incrementally with two additional commits since the last revision: - remove now useless comment - Rewrite unit test unresolved t2 too, cleanup JVM_CONSTANT_Class useless case - Changes: - all: https://git.openjdk.org/jdk/pull/14780/files - new: https://git.openjdk.org/jdk/pull/14780/files/c1a2d7c7..3784e653 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14780&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14780&range=03-04 Stats: 110 lines in 3 files changed: 7 ins; 80 del; 23 mod Patch: https://git.openjdk.org/jdk/pull/14780.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14780/head:pull/14780 PR: https://git.openjdk.org/jdk/pull/14780
Re: RFR: 8308762: Metaspace leak with Instrumentation.retransform [v6]
> Fix a small leak in constant pool merging during retransformation of a class. > If this class has a catch block with `Throwable`, the class `Throwable` is > pre-resolved in the constant pool, while all the other classes are in a > unresolved state. So the constant pool merging process was considering the > entry with pre-resolved class as different compared to the destination and > create a new entry. We now try to consider it as equal specially for > Methodref/Fieldref. Jean-Philippe Bempel has updated the pull request incrementally with one additional commit since the last revision: remove trailing whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/14780/files - new: https://git.openjdk.org/jdk/pull/14780/files/3784e653..26cb41b7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14780&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14780&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14780.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14780/head:pull/14780 PR: https://git.openjdk.org/jdk/pull/14780
Re: RFR: 8308762: Metaspace leak with Instrumentation.retransform
On Thu, 13 Jul 2023 14:34:38 GMT, Coleen Phillimore wrote: >> Fix a small leak in constant pool merging during retransformation of a >> class. If this class has a catch block with `Throwable`, the class >> `Throwable` is pre-resolved in the constant pool, while all the other >> classes are in a unresolved state. So the constant pool merging process was >> considering the entry with pre-resolved class as different compared to the >> destination and create a new entry. We now try to consider it as equal >> specially for Methodref/Fieldref. > > Also there is a nice test harness for class redefinition in the > test/hotspot/jtreg/serviceability/jvmti/RedefineClasses tests that you might > be able to use to add a test for this. @coleenp I have made the changes: cleanup code and rewrite the unit test - PR Comment: https://git.openjdk.org/jdk/pull/14780#issuecomment-1663642898
Re: RFR: JDK-8306441: Two phase segmented heap dump [v25]
On Wed, 2 Aug 2023 14:33:28 GMT, Yi Yang wrote:
>> ### Motivation and proposal
>> Hi, heap dump brings about pauses for application's execution(STW), this is
>> a well-known pain. JDK-8252842 have added parallel support to heapdump in an
>> attempt to alleviate this issue. However, all concurrent threads
>> competitively write heap data to the same file, and more memory is required
>> to maintain the concurrent buffer queue. In experiments, we did not feel a
>> significant performance improvement from that.
>>
>> The minor-pause solution, which is presented in this PR, is a two-phase
>> segmented heap dump:
>>
>> - Phase 1(STW): Concurrent threads directly write data to multiple heap
>> files.
>> - Phase 2(Non-STW): Merge multiple heap files into one complete heap dump
>> file. This process can happen outside safepoint.
>>
>> Now concurrent worker threads are not required to maintain a buffer queue,
>> which would result in more memory overhead, nor do they need to compete for
>> locks. The changes in the overall design are as follows:
>>
>> 
>> Fig1. Before
>>
>> 
>> Fig2. After this patch
>>
>> ### Performance evaluation
>> | memory | numOfThread | CompressionMode | STW | Total |
>> | ---| --- | --- | --- | |
>> | 8g | 1 T | N | 15.612 | 15.612 |
>> | 8g | 32 T | N | 2.561725 | 14.498 |
>> | 8g | 32 T | C1 | 2.3084878 | 14.198 |
>> | 8g | 32 T | C2 | 10.9355128 | 21.882 |
>> | 8g | 96 T | N | 2.6790452 | 14.012 |
>> | 8g | 96 T | C1 | 2.3044796 | 3.589 |
>> | 8g | 96 T | C2 | 9.7585151 | 20.219 |
>> | 16g | 1 T | N | 26.278 | 26.278 |
>> | 16g | 32 T | N | 5.231374 | 26.417 |
>> | 16g | 32 T | C1 | 5.6946983 | 6.538 |
>> | 16g | 32 T | C2 | 21.8211105 | 41.133 |
>> | 16g | 96 T | N | 6.2445556 | 27.141 |
>> | 16g | 96 T | C1 | 4.6007096 | 6.259 |
>> | 16g | 96 T | C2 | 19.2965783 | 39.007 |
>> | 32g | 1 T | N | 48.149 | 48.149 |
>> | 32g | 32 T | N | 10.7734677 | 61.643 |
>> | 32g | 32 T | C1 | 10.1642097 | 10.903 |
>> | 32g | 32 T | C2 | 43.8407607 | 88.152 |
>> | 32g | 96 T | N | 13.1522042 | 61.432 |
>> | 32g | 96 T | C1 | 9.0954641 | 9.885 |
>> | 32g | 96 T | C2 | 38.9900931 | 80.574 |
>> | 64g | 1 T | N | 100.583 | 100.583 |
>> | 64g | 32 T | N | 20.9233744 | 134.701 |
>> | 64g | 32 T | C1 | 18.5023784 | 19.358 |
>> | 64g | 32 T | C2 | 86.4748377 | 172.707 |
>> | 64g | 96 T | N | 26.7374116 | 126.08 |
>> | 64g | ...
>
> Yi Yang has updated the pull request incrementally with one additional commit
> since the last revision:
>
> whitespace
OK, nearly done!
We always log "parallelism true" which looks odd. It just means the dump path
has space for up to 9,999 log parts.
In VM_HeapDumper::doit() if we make it log whether it will try a parallel dump,
based on all the checks, it simplifies things I think.
Also the variable num_requested_dump_thread is not needed? It's just a copy of
_num_dumper_threads which we don't change.
Here is a suggestion for VM_HeapDumper::doit()
...
WorkerThreads* workers = ch->safepoint_workers();
uint num_active_workers = workers != nullptr ? workers->active_workers() : 0;
bool is_parallel = num_active_workers > 0 && can_parallel_dump() &&
is_parallel_dump();
log_info(heapdump)("Requested dump threads %u, active workers %u, parallelism
%s",
_num_dumper_threads, num_active_workers, is_parallel ?
"true" : "false");
if (!is_parallel) {
_num_dumper_threads = 1;
work(VMDumperWorkerId);
} else {
_num_dumper_threads = clamp(_num_dumper_threads, 2U, num_active_workers);
uint heap_only_dumper_threads = _num_dumper_threads - 1 /* VMDumper thread
*/;
_dumper_controller = new (std::nothrow)
DumperController(heap_only_dumper_threads);
ParallelObjectIterator poi(_num_dumper_threads);
_poi = &poi;
workers->run_task(this, _num_dumper_threads);
_poi = nullptr;
}
-
HeapDumpParallelTest.java
We need to change all of the:
Asserts.assertTrue(out.getStdout().contains(...
to:
out.shouldContain(...
...then we get the output included in the log if it fails, so we should see
what went wrong.
--
-
PR Comment: https://git.openjdk.org/jdk/pull/13667#issuecomment-1663979913
Re: RFR: 8312623: SA add NestHost and NestMembers attributes when dumping class
On Wed, 2 Aug 2023 05:07:31 GMT, David Holmes wrote: > Pity there is no regression/functional test for this. @dholmes-ora @plummercj I have improved [dumpclass tests](https://github.com/openjdk/jdk/commit/97618cc9bbadce4b51fc0fbee93557f4dcc8d26a) to cover up some cases for this PR and [JDK-8311971](https://bugs.openjdk.org/browse/JDK-8311971). If the tests makes sense I would like to include it in this PR. Is that okay? - PR Comment: https://git.openjdk.org/jdk/pull/15005#issuecomment-1664011833
Re: RFR: 8312623: SA add NestHost and NestMembers attributes when dumping class
On Wed, 2 Aug 2023 05:07:31 GMT, David Holmes wrote:
>> @dholmes-ora I verified the case for hidden dynamically injected classes.
>> The dumped class data for a hidden dynamically injected class does not have
>> any Nest-Host attribute. When generating these classes dynamically the VM
>> does not expose nest-host information in the class data, but sets the
>> nest-host directly in the InstanceKlass.
>>
>> Also verified the case for deeply nested classes by creating chain of nested
>> classes as:
>>
>> class DeepNest {
>> class NestLvl1 {
>> class NestLvl2 {
>> class NestLvl3 {
>> }
>> }
>> }
>> }
>>
>>
>> Only `DeepNest` has the `NestMembers` attribute which lists all the
>> NestLvl[1-3] classes. Rest all have `DeepNest` as the `NestHost` attribute.
>>
>> Does this cover all the cases you flagged?
>
> @ashu-mehra thanks for doing the additional testing. Pity there is no
> regression/functional test for this.
> @dholmes-ora @plummercj I have improved [dumpclass
> tests](https://github.com/openjdk/jdk/commit/97618cc9bbadce4b51fc0fbee93557f4dcc8d26a)
> to cover up some cases for this PR and
> [JDK-8311971](https://bugs.openjdk.org/browse/JDK-8311971). If the tests
> makes sense I would like to include it in this PR. Is that okay?
I think given the scope of the changes it would be better to do this with a
"Improve SA dumpclass testing" CR/PR.
-
PR Comment: https://git.openjdk.org/jdk/pull/15005#issuecomment-1664244491
Re: RFR: 8312623: SA add NestHost and NestMembers attributes when dumping class
On Thu, 3 Aug 2023 15:59:14 GMT, Chris Plummer wrote: >> @ashu-mehra thanks for doing the additional testing. Pity there is no >> regression/functional test for this. > >> @dholmes-ora @plummercj I have improved [dumpclass >> tests](https://github.com/openjdk/jdk/commit/97618cc9bbadce4b51fc0fbee93557f4dcc8d26a) >> to cover up some cases for this PR and >> [JDK-8311971](https://bugs.openjdk.org/browse/JDK-8311971). If the tests >> makes sense I would like to include it in this PR. Is that okay? > > I think given the scope of the changes it would be better to do this with a > "Improve SA dumpclass testing" CR/PR. @plummercj > I think given the scope of the changes it would be better to do this with a > "Improve SA dumpclass testing" CR/PR. Okay. I guess this can then be integrated as is. Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/15005#issuecomment-1664254792
Re: RFR: 8256379: Replace SIZE_FORMAT with 'z' length modifier
On Wed, 2 Aug 2023 01:31:43 GMT, Coleen Phillimore wrote:
> Replace SIZE_FORMAT with %zu and SIZE_FORMAT_X with 0x%zx using a 4 line sed
> script:
>
> sed -e 's/" SIZE_FORMAT "/%zu/g' -e 's/" SIZE_FORMAT,/%zu",/' \
> -e 's/print(SIZE_FORMAT "/print("%zu/g' \
> -e 's/" SIZE_FORMAT_X "/0x%zx/g' -e 's/" SIZE_FORMAT_X,/0x%zx",/g' \
> -e 's/" SIZE_FORMAT$/%zu"/' \
>$f >$f.new
>
> Minor fixups afterwards. I didn't change SIZE_FORMAT_W(number) because it
> would have required a better sed script.
>
> The definitions in globalDefinitions.hpp can be removed with a later PR, once
> all instances are cleaned out. This is partial so that people start using
> %zu instead.
>
> Tested with tier1 on Oracle supported platforms.
The %zu format looks much nicer than " SIZE_FORMAT " but I don't think we
should do this either. We can start using them with new code, or code we
modify.
-
PR Comment: https://git.openjdk.org/jdk/pull/15115#issuecomment-1664492414
Withdrawn: 8256379: Replace SIZE_FORMAT with 'z' length modifier
On Wed, 2 Aug 2023 01:31:43 GMT, Coleen Phillimore wrote:
> Replace SIZE_FORMAT with %zu and SIZE_FORMAT_X with 0x%zx using a 4 line sed
> script:
>
> sed -e 's/" SIZE_FORMAT "/%zu/g' -e 's/" SIZE_FORMAT,/%zu",/' \
> -e 's/print(SIZE_FORMAT "/print("%zu/g' \
> -e 's/" SIZE_FORMAT_X "/0x%zx/g' -e 's/" SIZE_FORMAT_X,/0x%zx",/g' \
> -e 's/" SIZE_FORMAT$/%zu"/' \
>$f >$f.new
>
> Minor fixups afterwards. I didn't change SIZE_FORMAT_W(number) because it
> would have required a better sed script.
>
> The definitions in globalDefinitions.hpp can be removed with a later PR, once
> all instances are cleaned out. This is partial so that people start using
> %zu instead.
>
> Tested with tier1 on Oracle supported platforms.
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.org/jdk/pull/15115
Re: RFR: 8256379: Replace SIZE_FORMAT with 'z' length modifier
On Wed, 2 Aug 2023 01:31:43 GMT, Coleen Phillimore wrote:
> Replace SIZE_FORMAT with %zu and SIZE_FORMAT_X with 0x%zx using a 4 line sed
> script:
>
> sed -e 's/" SIZE_FORMAT "/%zu/g' -e 's/" SIZE_FORMAT,/%zu",/' \
> -e 's/print(SIZE_FORMAT "/print("%zu/g' \
> -e 's/" SIZE_FORMAT_X "/0x%zx/g' -e 's/" SIZE_FORMAT_X,/0x%zx",/g' \
> -e 's/" SIZE_FORMAT$/%zu"/' \
>$f >$f.new
>
> Minor fixups afterwards. I didn't change SIZE_FORMAT_W(number) because it
> would have required a better sed script.
>
> The definitions in globalDefinitions.hpp can be removed with a later PR, once
> all instances are cleaned out. This is partial so that people start using
> %zu instead.
>
> Tested with tier1 on Oracle supported platforms.
I actually agree with using `%z` for this, but this seems an unusual way to
approach it. The `SIZE_FORMAT` is an abstraction which allows the actual format
specifier to be anything defined in `globalDefinitions.hpp`. Removing it to
hardcode a format thus seems like a bit of a backward step to me.
I've actually had a patch hanging around for the best part of a decade that
uses `%z` as the format specifier, but only on s390 (31-bit). As you may know,
`size_t` and `int` aren't interchangeable on s390, which used to throw up all
sorts of errors with the casts in HotSpot. On that platform, it really is
necessary to use `%z` for the size format.
The patch pretty much got dropped on the floor as we didn't build on s390 on
newer JDKs and obviously we'd need to start there to get it upstream and then
backport. The actual patch was pretty simple though:
~~~
diff --git openjdk.orig/hotspot/src/share/vm/utilities/globalDefinitions.hpp
openjdk/hotspot/src/share/vm/utilities/globalDefinitions.hpp
--- openjdk.orig/hotspot/src/share/vm/utilities/globalDefinitions.hpp
+++ openjdk/hotspot/src/share/vm/utilities/globalDefinitions.hpp
@@ -1389,12 +1389,21 @@
#define INTPTR_FORMAT_W(width) "%" #width PRIxPTR
+#if defined(S390) && !defined(_LP64)
+#define SSIZE_FORMAT "%z" PRIdPTR
+#define SIZE_FORMAT "%z" PRIuPTR
+#define SIZE_FORMAT_HEX "0x%z" PRIxPTR
+#define SSIZE_FORMAT_W(width) "%" #width "z" PRIdPTR
+#define SIZE_FORMAT_W(width) "%" #width "z" PRIuPTR
+#define SIZE_FORMAT_HEX_W(width) "0x%" #width "z" PRIxPTR
+#else // !S390
#define SSIZE_FORMAT "%" PRIdPTR
#define SIZE_FORMAT "%" PRIuPTR
#define SIZE_FORMAT_HEX "0x%" PRIxPTR
#define SSIZE_FORMAT_W(width) "%" #width PRIdPTR
#define SIZE_FORMAT_W(width) "%" #width PRIuPTR
#define SIZE_FORMAT_HEX_W(width) "0x%" #width PRIxPTR
+#endif // S390
#define INTX_FORMAT "%" PRIdPTR
#define UINTX_FORMAT "%" PRIuPTR
~~~
plus a bunch of cases where the wrong format was being used in the `printf`
statements (likely resolved now anyway; this was against 8u)
I think you could do something similar and include backwards compatibility by
detecting whether `%z` is usable during `configure` and then adding a similar
`#ifdef` but based on something defined by `configure`.
I'd be happy to propose something if you don't already have an alternate
solution in the works.
-
PR Comment: https://git.openjdk.org/jdk/pull/15115#issuecomment-1664765034
Re: RFR: JDK-8306441: Two phase segmented heap dump [v25]
On Thu, 3 Aug 2023 13:24:19 GMT, Kevin Walls wrote:
> Also the variable num_requested_dump_thread is not needed? It's just a copy
> of _num_dumper_threads which we don't change.
This is needed because _num_dumper_threads will change later, the requested
dump thread may not equal to actual _num_dumper_threads. `is_parallel_dump()`
checks _num_dump_thread, so we cannot use suggested code.
This involves code style favor, to avoid futile effort, do you think the below
code is acceptable?
--- a/src/hotspot/share/services/heapDumper.cpp
+++ b/src/hotspot/share/services/heapDumper.cpp
@@ -1724,15 +1724,8 @@ class VM_HeapDumper : public VM_GC_Operation, public
WorkerTask {
}
int dump_seq() { return _dump_seq; }
bool is_parallel_dump() { return _num_dumper_threads > 1; }
- bool can_parallel_dump() {
-const char* base_path = writer()->get_file_path();
-assert(base_path != nullptr, "sanity check");
-if ((strlen(base_path) + 7/*.p\d\d\d\d\0*/) >= JVM_MAXPATHLEN) {
- // no extra path room for separate heap dump files
- return false;
-}
-return true;
- }
+ bool can_parallel_dump(uint num_active_workers);
+
VMOp_Type type() const { return VMOp_HeapDumper; }
virtual bool doit_prologue();
void doit();
@@ -1923,6 +1916,33 @@ bool VM_HeapDumper::doit_prologue() {
return VM_GC_Operation::doit_prologue();
}
+bool VM_HeapDumper::can_parallel_dump(uint num_active_workers) {
+ assert(base_path != nullptr, "sanity check");
+ bool has_extra_path_room = true;
+ const char* base_path = writer()->get_file_path();
+ if ((strlen(base_path) + 7/*.p\d\d\d\d\0*/) >= JVM_MAXPATHLEN) {
+// no extra path room for separate heap dump files
+has_extra_path_room = false;
+ }
+
+ bool can_parallel = true;
+ uint num_requested_dump_thread = _num_dumper_threads;
+ if (num_active_workers <= 1 || // serial gc?
+ num_requested_dump_thread <= 1 || // request serial dump?
+ !has_extra_path_room) { // has extra path room for segmented
dump files?
+_num_dumper_threads = 1;
+can_parallel = false;
+ } else {
+_num_dumper_threads = clamp(num_requested_dump_thread, 2U,
num_active_workers);
+can_parallel = true;
+ }
+
+ log_info(heapdump)("Requested dump threads %u, active dump threads %u, "
+ "actual dump threads %u, parallelism %s",
+ num_requested_dump_thread, num_active_workers,
+ _num_dumper_threads, can_parallel ? "true" : "false");
+ return can_parallel;
+}
// The VM operation that dumps the heap. The dump consists of the following
// records:
@@ -1970,20 +1990,10 @@ void VM_HeapDumper::doit() {
WorkerThreads* workers = ch->safepoint_workers();
uint num_active_workers = workers != nullptr ? workers->active_workers() : 0;
- uint num_requested_dump_thread = _num_dumper_threads;
- bool can_parallel = can_parallel_dump();
- log_info(heapdump)("Requested dump threads %u, active dump threads %u, "
"parallelism %s",
- num_requested_dump_thread, num_active_workers,
can_parallel ? "true" : "false");
- if (num_active_workers <= 1 || // serial gc?
- num_requested_dump_thread <= 1 || // request serial dump?
- !can_parallel) {// can not dump in parallel?
-// Use serial dump, set dumper threads and writer threads number to 1.
-_num_dumper_threads = 1;
+ if (can_parallel_dump(num_active_workers)) {
work(VMDumperWorkerId);
} else {
-// Use parallel dump otherwise
-_num_dumper_threads = clamp(num_requested_dump_thread, 2U,
num_active_workers);
uint heap_only_dumper_threads = _num_dumper_threads - 1 /* VMDumper thread
*/;
_dumper_controller = new (std::nothrow)
DumperController(heap_only_dumper_threads);
ParallelObjectIterator poi(_num_dumper_threads);
> out.shouldContain(..then we get the output included in the log if it
> fails, so we should see what went wrong.
Acknowledge
-
PR Comment: https://git.openjdk.org/jdk/pull/13667#issuecomment-1664913578
Re: RFR: 8256379: Replace SIZE_FORMAT with 'z' length modifier
On Thu, 3 Aug 2023 23:39:11 GMT, Andrew John Hughes wrote: > Removing it to hardcode a format thus seems like a bit of a backward step to > me. But we only defined abstract things like `SIZE_FORMAT` because there was no single specifier accepted by all C runtimes and which behaves in the same (or similar enough) way. Once we have use of `%zu` or %zd` available everywhere then we no longer need the abstraction. - PR Comment: https://git.openjdk.org/jdk/pull/15115#issuecomment-1665059059
Re: RFR: 8307462: [REDO] VmObjectAlloc is not generated by intrinsics methods which allocate objects [v2]
> The fix adds posting VmObjectAlloc events by Unsafe.allocateInstance(Class > cls). The previous attempt to post event directly from > 'LibraryCallKit::inline_unsafe_allocate()' cause performance regression even > if jvmti event is not enabled. Some optimizations have been disabled just > because possible usage and escaping of newly allocated object. > So event posting is doing by returning to interpreter if events are enabled. > > I verified that that performance (run locally only) of > org.renaissance.jdk.streams.JmhScrabble.runOperation > doesn't change if events are not enabled. > > There might be other intrinsics like > 'LibraryCallKit::inline_unsafe_newArray()' where VM allocate memory. I'm > going to file separate issue to find and fix them. > > Many thanks to Tobias H. for proposed solution. > > Testing with all tiers. Leonid Mesnik has updated the pull request incrementally with one additional commit since the last revision: combined jvmti code - Changes: - all: https://git.openjdk.org/jdk/pull/15110/files - new: https://git.openjdk.org/jdk/pull/15110/files/07499679..5095114f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15110&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15110&range=00-01 Stats: 38 lines in 1 file changed: 17 ins; 21 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15110.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15110/head:pull/15110 PR: https://git.openjdk.org/jdk/pull/15110
