Re: RFR: 8252842: Extend jmap to support parallel heap dump [v32]
On Mon, 6 Sep 2021 08:03:22 GMT, Lin Zang wrote: >> 8252842: Extend jmap to support parallel heap dump > > Lin Zang has updated the pull request incrementally with one additional > commit since the last revision: > > fix build error I will start reviewing this today. - PR: https://git.openjdk.java.net/jdk/pull/2261
Re: RFR: 8265489: Stress test times out because of long ObjectSynchronizer::monitors_iterate(...) operation
On Thu, 19 Aug 2021 21:18:53 GMT, Leonid Mesnik wrote: > monitors_iterate make several checks which often are true before filter > monitor by a thread. It might take a lot of time when there are a lot of > threads. So it makes sense to first check thread and only then other > conditions. Moving the thread check from the closure's do_monitor() call into monitors_iterate() as early as possible is a good idea. Do you have any measurements to show how much this helps? I'm okay if you don't and I'd be happy waiting to see if it makes a difference with some of those Tier8 timeouts... - Marked as reviewed by dcubed (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5194
Re: RFR: 8265489: Stress test times out because of long ObjectSynchronizer::monitors_iterate(...) operation
On Fri, 3 Sep 2021 01:26:01 GMT, Daniel D. Daugherty wrote:
>> monitors_iterate make several checks which often are true before filter
>> monitor by a thread. It might take a lot of time when there are a lot of
>> threads. So it makes sense to first check thread and only then other
>> conditions.
>
> src/hotspot/share/runtime/synchronizer.cpp line 981:
>
>> 979: if (mid->owner() != thread) {
>> 980: return;
>> 981: }
>
> The `iter` is processing the in-use-list and you're bailing the iteration
> when you run into an ObjectMonitor that is not owned by `thread`, but
> that doesn't mean that there's not an ObjectMonitor owned by `thread`
> later on in the in-use-list.
>
> So I could see you doing a `continue` here, but not a `return`.
Thanks for resolving the above comment.
-
PR: https://git.openjdk.java.net/jdk/pull/5194
Re: Integrated: 8273047: test jfr/api/consumer/TestRecordedFrame.java timing out
On Wed, 8 Sep 2021 02:07:47 GMT, David Holmes wrote: >> A trivial fix to bump the timeout value for the second sub-test in >> jfr/api/consumer/TestRecordedFrame.java. See the bug report >> for the gory details. > > LGTM! > > Thanks, > David @dholmes-ora - Thanks for the lightning fast review! - PR: https://git.openjdk.java.net/jdk/pull/5405
Integrated: 8273047: test jfr/api/consumer/TestRecordedFrame.java timing out
On Wed, 8 Sep 2021 02:04:50 GMT, Daniel D. Daugherty wrote: > A trivial fix to bump the timeout value for the second sub-test in > jfr/api/consumer/TestRecordedFrame.java. See the bug report > for the gory details. This pull request has now been integrated. Changeset: ea4907a8 Author:Daniel D. Daugherty URL: https://git.openjdk.java.net/jdk/commit/ea4907a8789e00f9ec8d4175241246b8cf53f3f6 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8273047: test jfr/api/consumer/TestRecordedFrame.java timing out Reviewed-by: dholmes - PR: https://git.openjdk.java.net/jdk/pull/5405
Integrated: 8273047: test jfr/api/consumer/TestRecordedFrame.java timing out
A trivial fix to bump the timeout value for the second sub-test in jfr/api/consumer/TestRecordedFrame.java. See the bug report for the gory details. - Commit messages: - 8273047: test jfr/api/consumer/TestRecordedFrame.java timing out Changes: https://git.openjdk.java.net/jdk/pull/5405/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5405&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273047 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5405.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5405/head:pull/5405 PR: https://git.openjdk.java.net/jdk/pull/5405
Re: Integrated: 8273047: test jfr/api/consumer/TestRecordedFrame.java timing out
On Wed, 8 Sep 2021 02:04:50 GMT, Daniel D. Daugherty wrote: > A trivial fix to bump the timeout value for the second sub-test in > jfr/api/consumer/TestRecordedFrame.java. See the bug report > for the gory details. LGTM! Thanks, David - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5405
Re: RFR: 8269537: memset() is called after operator new
On Tue, 7 Sep 2021 12:25:54 GMT, Leo Korinth wrote:
> The basic problem is that we are relying on undefined behaviour, as
> documented in the code:
>
> // This whole business of passing information from ResourceObj::operator new
> // to the ResourceObj constructor via fields in the "object" is technically
> UB.
> // But it seems to work within the limitations of HotSpot usage (such as no
> // multiple inheritance) with the compilers and compiler options we're using.
> // And it gives some possibly useful checking for misuse of ResourceObj.
>
>
> I am removing the undefined behaviour by passing the type of allocation
> through a thread local variable.
>
> This solution has some advantages:
> 1) it is not UB
> 2) it is simpler and easier to understand
> 3) it uses less memory (I could make it use even less if I made the enum
> `allocation_type` a u8)
> 4) in the *very* unlikely situation that stack memory (or embedded) already
> equals the data calculated from the address of the object, the code will also
> work.
>
> When doing the change, I also updated `allocated_on_stack()` to the new name
> `allocated_on_stack_or_embedded()` which is much harder to misinterpret.
>
> I also disallow to "fake" the memory type by explicitly calling
> `ResourceObj::set_allocation_type`.
>
> This forced me to change two places that is faking the allocation type of an
> embedded `GrowableArray` from `STACK_OR_EMBEDDED` to `C_HEAP`. The faking of
> the type is hard to understand as a `STACK_OR_EMBEDDED` `GrowableArray` can
> allocate any type of object. My guess is that `GrowableArray` has changed
> behaviour, or maybe that it was hard to understand because the old naming of
> `allocated_on_stack()`.
>
> I have also tried to update the comments. In doing that I not only changed
> the comments for this change, but also for the *incorrect* advice to always
> delete object you allocate with new.
>
> Testing on debug build tier1-3
> Testing on release build tier1
Changes requested by iklam (Reviewer).
src/hotspot/share/memory/allocation.hpp line 439:
> 437: void* operator new(size_t size, const std::nothrow_t&
> nothrow_constant) throw() {
> 438: address res = (address)resource_allocate_bytes(size,
> AllocFailStrategy::RETURN_NULL);
> 439: DEBUG_ONLY(if (res != NULL) _thread_last_allocated =
> RESOURCE_AREA;)
Maybe we should also guard against the possibility of nested allocations, which
may trash `_thread_last_allocated`?
#define PUSH_RESOURCE_OBJ_ALLOC_TYPE(t) \
assert(_thread_last_allocated == STACK_OR_EMBEDDED, "must not be nested"); \
DEBUG_ONLY(_thread_last_allocated = t); \
...
if (res != NULL) {
PUSH_RESOURCE_OBJ_ALLOC_TYPE(RESOURCE_AREA);
}
Similarly, the `ResourceObj` constructor should use a corresponding
`POP_RESOURCE_OBJ_ALLOC_TYPE` macro.
-
PR: https://git.openjdk.java.net/jdk/pull/5387
Re: RFR: 8252842: Extend jmap to support parallel heap dump
On Thu, 28 Jan 2021 07:30:29 GMT, Serguei Spitsyn wrote: >> 8252842: Extend jmap to support parallel heap dump > > Hi Lin, > It is also in my memory that you actually did not have 4 arguments. > The real incompatibility issue was that the order of arguments was swapped. > It is why it was relatively easy to fall back and just update the constants > with 3 instead of 4 and swap the order of arguments back. > Thanks, > Serguei @sspitsyn or @schmelter-sap Can one of you provide a second review for this change. The GC team would like to start a review for changes that will heavily impact this same code, so it would be best if these changes could be pushed first. - PR: https://git.openjdk.java.net/jdk/pull/2261
RFR: 8269537: memset() is called after operator new
The basic problem is that we are relying on undefined behaviour, as documented in the code: // This whole business of passing information from ResourceObj::operator new // to the ResourceObj constructor via fields in the "object" is technically UB. // But it seems to work within the limitations of HotSpot usage (such as no // multiple inheritance) with the compilers and compiler options we're using. // And it gives some possibly useful checking for misuse of ResourceObj. I am removing the undefined behaviour by passing the type of allocation through a thread local variable. This solution has some advantages: 1) it is not UB 2) it is simpler and easier to understand 3) it uses less memory (I could make it use even less if I made the enum `allocation_type` a u8) 4) in the *very* unlikely situation that stack memory (or embedded) already equals the data calculated from the address of the object, the code will also work. When doing the change, I also updated `allocated_on_stack()` to the new name `allocated_on_stack_or_embedded()` which is much harder to misinterpret. I also disallow to "fake" the memory type by explicitly calling `ResourceObj::set_allocation_type`. This forced me to change two places that is faking the allocation type of an embedded `GrowableArray` from `STACK_OR_EMBEDDED` to `C_HEAP`. The faking of the type is hard to understand as a `STACK_OR_EMBEDDED` `GrowableArray` can allocate any type of object. My guess is that `GrowableArray` has changed behaviour, or maybe that it was hard to understand because the old naming of `allocated_on_stack()`. I have also tried to update the comments. In doing that I not only changed the comments for this change, but also for the *incorrect* advice to always delete object you allocate with new. Testing on debug build tier1-3 Testing on release build tier1 - Commit messages: - 8269537: memset() is called after operator new Changes: https://git.openjdk.java.net/jdk/pull/5387/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5387&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269537 Stats: 109 lines in 8 files changed: 1 ins; 66 del; 42 mod Patch: https://git.openjdk.java.net/jdk/pull/5387.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5387/head:pull/5387 PR: https://git.openjdk.java.net/jdk/pull/5387
