RFR: 8261473: Shenandoah: Add breakpoint suppoprt

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8261473: Shenandoah: Add breakpoint suppoprt

Zhengyu Gu-3
Please review this patch that adds breakpoint support for Shenandoah, that allows Shenandoah to access a few tests:

gc/TestConcurrentGCBreakpoints.java
gc/TestJNIWeak/TestJNIWeak.java
gc/TestReferenceClearDuringMarking.java
gc/TestReferenceClearDuringReferenceProcessing.java
gc/TestReferenceRefersTo.java

The drawback is that above tests can not run with passive mode, which can result tests to hang, as breakpoints only apply to concurrent GC.

Test:
- [x] hotspot_gc_shenandoah
- [x] tier1 with Shenandoah

-------------

Commit messages:
 - update
 - init update

Changes: https://git.openjdk.java.net/jdk/pull/2489/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2489&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261473
  Stats: 170 lines in 8 files changed: 158 ins; 2 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2489.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2489/head:pull/2489

PR: https://git.openjdk.java.net/jdk/pull/2489
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261473: Shenandoah: Add breakpoint suppoprt [v2]

Zhengyu Gu-3
> Please review this patch that adds breakpoint support for Shenandoah, that allows Shenandoah to access a few tests:
>
> gc/TestConcurrentGCBreakpoints.java
> gc/TestJNIWeak/TestJNIWeak.java
> gc/TestReferenceClearDuringMarking.java
> gc/TestReferenceClearDuringReferenceProcessing.java
> gc/TestReferenceRefersTo.java
>
> The drawback is that above tests can not run with passive mode, which can result tests to hang, as breakpoints only apply to concurrent GC.
>
> Test:
> - [x] hotspot_gc_shenandoah
> - [x] tier1 with Shenandoah

Zhengyu Gu has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:

 - Merge
 - update
 - init update

-------------

Changes: https://git.openjdk.java.net/jdk/pull/2489/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2489&range=01
  Stats: 170 lines in 8 files changed: 158 ins; 2 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2489.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2489/head:pull/2489

PR: https://git.openjdk.java.net/jdk/pull/2489
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261473: Shenandoah: Add breakpoint suppoprt [v2]

Roman Kennke
On Wed, 10 Feb 2021 20:13:52 GMT, Zhengyu Gu <[hidden email]> wrote:

>> Please review this patch that adds breakpoint support for Shenandoah, that allows Shenandoah to access a few tests:
>>
>> gc/TestConcurrentGCBreakpoints.java
>> gc/TestJNIWeak/TestJNIWeak.java
>> gc/TestReferenceClearDuringMarking.java
>> gc/TestReferenceClearDuringReferenceProcessing.java
>> gc/TestReferenceRefersTo.java
>>
>> The drawback is that above tests can not run with passive mode, which can result tests to hang, as breakpoints only apply to concurrent GC.
>>
>> Test:
>> - [x] hotspot_gc_shenandoah
>> - [x] tier1 with Shenandoah
>
> Zhengyu Gu has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - Merge
>  - update
>  - init update

The change looks good to me, thanks!

-------------

Marked as reviewed by rkennke (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2489
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261473: Shenandoah: Add breakpoint suppoprt [v2]

Aleksey Shipilev-5
In reply to this post by Zhengyu Gu-3
On Wed, 10 Feb 2021 20:13:52 GMT, Zhengyu Gu <[hidden email]> wrote:

>> Please review this patch that adds breakpoint support for Shenandoah, that allows Shenandoah to access a few tests:
>>
>> gc/TestConcurrentGCBreakpoints.java
>> gc/TestJNIWeak/TestJNIWeak.java
>> gc/TestReferenceClearDuringMarking.java
>> gc/TestReferenceClearDuringReferenceProcessing.java
>> gc/TestReferenceRefersTo.java
>>
>> The drawback is that above tests can not run with passive mode, which can result tests to hang, as breakpoints only apply to concurrent GC.
>>
>> Test:
>> - [x] hotspot_gc_shenandoah
>> - [x] tier1 with Shenandoah
>
> Zhengyu Gu has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
>  - Merge
>  - update
>  - init update

Looks fine, minor nits.

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 52:

> 50:
> 51: // Breakpoint support
> 52: class ShenandoahConcurrentGCScope : public StackObj {

Let's call these `ShenandoahGCBreakpointScope` and `ShenandoahMarkBreakpointScope`?

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 476:

> 474: bool ShenandoahControlThread::is_async_gc(GCCause::Cause cause) const {
> 475:   return cause == GCCause::_wb_breakpoint;
> 476: }

Do we really need this method? What is "async gc" anyway? I think you can just inline the method at its only use.

-------------

Marked as reviewed by shade (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2489
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261473: Shenandoah: Add breakpoint suppoprt [v3]

Zhengyu Gu-3
In reply to this post by Zhengyu Gu-3
> Please review this patch that adds breakpoint support for Shenandoah, that allows Shenandoah to access a few tests:
>
> gc/TestConcurrentGCBreakpoints.java
> gc/TestJNIWeak/TestJNIWeak.java
> gc/TestReferenceClearDuringMarking.java
> gc/TestReferenceClearDuringReferenceProcessing.java
> gc/TestReferenceRefersTo.java
>
> The drawback is that above tests can not run with passive mode, which can result tests to hang, as breakpoints only apply to concurrent GC.
>
> Test:
> - [x] hotspot_gc_shenandoah
> - [x] tier1 with Shenandoah

Zhengyu Gu has updated the pull request incrementally with one additional commit since the last revision:

  Shade's comments and fixing a merge error

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2489/files
  - new: https://git.openjdk.java.net/jdk/pull/2489/files/2b88f7a6..4f19c4cd

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2489&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2489&range=01-02

  Stats: 18 lines in 3 files changed: 2 ins; 7 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2489.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2489/head:pull/2489

PR: https://git.openjdk.java.net/jdk/pull/2489
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261473: Shenandoah: Add breakpoint suppoprt [v2]

Zhengyu Gu-3
In reply to this post by Aleksey Shipilev-5
On Thu, 18 Feb 2021 13:19:18 GMT, Aleksey Shipilev <[hidden email]> wrote:

>> Zhengyu Gu has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>>
>>  - Merge
>>  - update
>>  - init update
>
> src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 52:
>
>> 50:
>> 51: // Breakpoint support
>> 52: class ShenandoahConcurrentGCScope : public StackObj {
>
> Let's call these `ShenandoahGCBreakpointScope` and `ShenandoahMarkBreakpointScope`?

Done

> src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 476:
>
>> 474: bool ShenandoahControlThread::is_async_gc(GCCause::Cause cause) const {
>> 475:   return cause == GCCause::_wb_breakpoint;
>> 476: }
>
> Do we really need this method? What is "async gc" anyway? I think you can just inline the method at its only use.

Done

-------------

PR: https://git.openjdk.java.net/jdk/pull/2489
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261473: Shenandoah: Add breakpoint suppoprt [v3]

Aleksey Shipilev-5
In reply to this post by Zhengyu Gu-3
On Thu, 18 Feb 2021 14:06:54 GMT, Zhengyu Gu <[hidden email]> wrote:

>> Please review this patch that adds breakpoint support for Shenandoah, that allows Shenandoah to access a few tests:
>>
>> gc/TestConcurrentGCBreakpoints.java
>> gc/TestJNIWeak/TestJNIWeak.java
>> gc/TestReferenceClearDuringMarking.java
>> gc/TestReferenceClearDuringReferenceProcessing.java
>> gc/TestReferenceRefersTo.java
>>
>> The drawback is that above tests can not run with passive mode, which can result tests to hang, as breakpoints only apply to concurrent GC.
>>
>> Test:
>> - [x] hotspot_gc_shenandoah
>> - [x] tier1 with Shenandoah
>
> Zhengyu Gu has updated the pull request incrementally with one additional commit since the last revision:
>
>   Shade's comments and fixing a merge error

Marked as reviewed by shade (Reviewer).

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp line 52:

> 50:
> 51: // Breakpoint support
> 52: class ShenandoahBreakpointScope : public StackObj {

Should probably be `ShenandoahBreakpointGCScope` to match that other "MarkScope".

-------------

PR: https://git.openjdk.java.net/jdk/pull/2489
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261473: Shenandoah: Add breakpoint suppoprt [v4]

Zhengyu Gu-3
In reply to this post by Zhengyu Gu-3
> Please review this patch that adds breakpoint support for Shenandoah, that allows Shenandoah to access a few tests:
>
> gc/TestConcurrentGCBreakpoints.java
> gc/TestJNIWeak/TestJNIWeak.java
> gc/TestReferenceClearDuringMarking.java
> gc/TestReferenceClearDuringReferenceProcessing.java
> gc/TestReferenceRefersTo.java
>
> The drawback is that above tests can not run with passive mode, which can result tests to hang, as breakpoints only apply to concurrent GC.
>
> Test:
> - [x] hotspot_gc_shenandoah
> - [x] tier1 with Shenandoah

Zhengyu Gu has updated the pull request incrementally with one additional commit since the last revision:

  More renaming per @shade

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2489/files
  - new: https://git.openjdk.java.net/jdk/pull/2489/files/4f19c4cd..a9629c8f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2489&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2489&range=02-03

  Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2489.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2489/head:pull/2489

PR: https://git.openjdk.java.net/jdk/pull/2489
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261473: Shenandoah: Add breakpoint suppoprt [v4]

Aleksey Shipilev-5
On Thu, 18 Feb 2021 15:52:56 GMT, Zhengyu Gu <[hidden email]> wrote:

>> Please review this patch that adds breakpoint support for Shenandoah, that allows Shenandoah to access a few tests:
>>
>> gc/TestConcurrentGCBreakpoints.java
>> gc/TestJNIWeak/TestJNIWeak.java
>> gc/TestReferenceClearDuringMarking.java
>> gc/TestReferenceClearDuringReferenceProcessing.java
>> gc/TestReferenceRefersTo.java
>>
>> The drawback is that above tests can not run with passive mode, which can result tests to hang, as breakpoints only apply to concurrent GC.
>>
>> Test:
>> - [x] hotspot_gc_shenandoah
>> - [x] tier1 with Shenandoah
>
> Zhengyu Gu has updated the pull request incrementally with one additional commit since the last revision:
>
>   More renaming per @shade

Marked as reviewed by shade (Reviewer).

-------------

PR: https://git.openjdk.java.net/jdk/pull/2489
Reply | Threaded
Open this post in threaded view
|

Integrated: 8261473: Shenandoah: Add breakpoint support

Zhengyu Gu-3
In reply to this post by Zhengyu Gu-3
On Tue, 9 Feb 2021 21:19:11 GMT, Zhengyu Gu <[hidden email]> wrote:

> Please review this patch that adds breakpoint support for Shenandoah, that allows Shenandoah to access a few tests:
>
> gc/TestConcurrentGCBreakpoints.java
> gc/TestJNIWeak/TestJNIWeak.java
> gc/TestReferenceClearDuringMarking.java
> gc/TestReferenceClearDuringReferenceProcessing.java
> gc/TestReferenceRefersTo.java
>
> The drawback is that above tests can not run with passive mode, which can result tests to hang, as breakpoints only apply to concurrent GC.
>
> Test:
> - [x] hotspot_gc_shenandoah
> - [x] tier1 with Shenandoah

This pull request has now been integrated.

Changeset: 9cf4f90d
Author:    Zhengyu Gu <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/9cf4f90d
Stats:     165 lines in 7 files changed: 153 ins; 2 del; 10 mod

8261473: Shenandoah: Add breakpoint support

Reviewed-by: rkennke, shade

-------------

PR: https://git.openjdk.java.net/jdk/pull/2489