RFR: JDK-8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841

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

RFR: JDK-8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841

Niklas Radomski
Introduces support for _nmethod entry barriers_ and _c2i entry barriers_ on the ppc platform. Those are required to enable concurrent class unloading for compatible garbage collectors, such as Shenandoah or zGC.

_This is a preparational change for the Shenandoah GC port to ppc. As such, it introduces features that the current version doesn't make use of, but that are required for the upcoming change. This way, the scope of the upcoming change is limited to Shenandoah-specific functionality; making its review a little easier._

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

Commit messages:
 - Remove extraneous whitespace
 - [PPC64] Introduce nmethod_entry_barrier and c2i_entry_barrier

Changes: https://git.openjdk.java.net/jdk/pull/2432/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2432&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8260372
  Stats: 264 lines in 10 files changed: 254 ins; 0 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2432.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2432/head:pull/2432

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

Re: RFR: JDK-8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841

Martin Doerr
On Fri, 5 Feb 2021 18:04:34 GMT, Niklas Radomski <[hidden email]> wrote:

> Introduces support for _nmethod entry barriers_ and _c2i entry barriers_ on the ppc platform. Those are required to enable concurrent class unloading for compatible garbage collectors, such as Shenandoah or zGC.
>
> _This is a preparational change for the Shenandoah GC port to ppc. As such, it introduces features that the current version doesn't make use of, but that are required for the upcoming change. This way, the scope of the upcoming change is limited to Shenandoah-specific functionality; making its review a little easier._

Thanks for the contribution. Looks very good. I only have a few minor requests.

src/hotspot/cpu/ppc/gc/shared/barrierSetNMethod_ppc.cpp line 44:

> 42:
> 43: public:
> 44:   unsigned short get_guard_value() const {

Return type should be int.

src/hotspot/cpu/ppc/gc/shared/barrierSetNMethod_ppc.cpp line 49:

> 47:   }
> 48:
> 49:   void release_set_guard_value(unsigned short value) {

Parameter should be int.

src/hotspot/cpu/ppc/gc/shared/barrierSetNMethod_ppc.cpp line 113:

> 111:   // Nothing to do.
> 112:   // Unlike other platforms, the frame resolution is done in the nmethod entry barrier stub.
> 113:   // This way, writing frame information on the stack can be avoided.

[optional] I'd replace the last sentence:
// PPC64 always has a valid back chain so the stub can simply pop the frame and there's nothing to do here.

src/hotspot/cpu/ppc/stubGenerator_ppc.cpp line 3581:

> 3579:
> 3580:     // Return to prologue if no deoptimization is required (bnelr)
> 3581:     __ bclr(Assembler::bcondCRbiIs1_bhintIsTaken, Assembler::bi0(CCR0, Assembler::equal), Assembler::bhintIsTaken);

Branch hint is provided twice. Not an error, but bcondCRbiIs1 would be cleaner if you keep it at the end.

src/hotspot/cpu/ppc/stubGenerator_ppc.cpp line 3592:

> 3590:
> 3591:     // Restore link register. Points to an instruction in the previous Java frame; effectively resuming
> 3592:     // its execution after this method's deoptimization.  This method's prologue is aborted.

Unprecise: We're not resuming in the caller after deoptimization, we call handle_wrong_method which finds a better callee (e.g. interpreter) and jumps to it.

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

Changes requested by mdoerr (Reviewer).

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

Re: RFR: JDK-8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841 [v2]

Niklas Radomski
In reply to this post by Niklas Radomski
> Introduces support for _nmethod entry barriers_ and _c2i entry barriers_ on the ppc platform. Those are required to enable concurrent class unloading for compatible garbage collectors, such as Shenandoah or zGC.
>
> _This is a preparational change for the Shenandoah GC port to ppc. As such, it introduces features that the current version doesn't make use of, but that are required for the upcoming change. This way, the scope of the upcoming change is limited to Shenandoah-specific functionality; making its review a little easier._

Niklas Radomski has updated the pull request incrementally with one additional commit since the last revision:

  Apply Martin's feedback

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2432/files
  - new: https://git.openjdk.java.net/jdk/pull/2432/files/b6add58b..ab354b68

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

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

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

Re: RFR: JDK-8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841 [v2]

Martin Doerr
On Sun, 7 Feb 2021 22:02:04 GMT, Niklas Radomski <[hidden email]> wrote:

>> Introduces support for _nmethod entry barriers_ and _c2i entry barriers_ on the ppc platform. Those are required to enable concurrent class unloading for compatible garbage collectors, such as Shenandoah or zGC.
>>
>> _This is a preparational change for the Shenandoah GC port to ppc. As such, it introduces features that the current version doesn't make use of, but that are required for the upcoming change. This way, the scope of the upcoming change is limited to Shenandoah-specific functionality; making its review a little easier._
>
> Niklas Radomski has updated the pull request incrementally with one additional commit since the last revision:
>
>   Apply Martin's feedback

Thanks for the update. I found an additional Big Endian bug (missing function descriptor).

src/hotspot/cpu/ppc/stubGenerator_ppc.cpp line 3556:

> 3554:     StubCodeMark mark(this, "StubRoutines", "nmethod_entry_barrier");
> 3555:
> 3556:     address stub_address = __ pc();

Please use __ function_entry(); instead of __ pc();
Otherwise it's broken on Big Endian which use ABI v1.

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

Changes requested by mdoerr (Reviewer).

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

Re: RFR: JDK-8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841 [v2]

Martin Doerr
In reply to this post by Niklas Radomski
On Sun, 7 Feb 2021 22:02:04 GMT, Niklas Radomski <[hidden email]> wrote:

>> Introduces support for _nmethod entry barriers_ and _c2i entry barriers_ on the ppc platform. Those are required to enable concurrent class unloading for compatible garbage collectors, such as Shenandoah or zGC.
>>
>> _This is a preparational change for the Shenandoah GC port to ppc. As such, it introduces features that the current version doesn't make use of, but that are required for the upcoming change. This way, the scope of the upcoming change is limited to Shenandoah-specific functionality; making its review a little easier._
>
> Niklas Radomski has updated the pull request incrementally with one additional commit since the last revision:
>
>   Apply Martin's feedback

Marked as reviewed by mdoerr (Reviewer).

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

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

Re: RFR: JDK-8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841 [v2]

Martin Doerr
In reply to this post by Martin Doerr
On Mon, 8 Feb 2021 11:23:03 GMT, Martin Doerr <[hidden email]> wrote:

>> Niklas Radomski has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Apply Martin's feedback
>
> src/hotspot/cpu/ppc/stubGenerator_ppc.cpp line 3556:
>
>> 3554:     StubCodeMark mark(this, "StubRoutines", "nmethod_entry_barrier");
>> 3555:
>> 3556:     address stub_address = __ pc();
>
> Please use __ function_entry(); instead of __ pc();
> Otherwise it's broken on Big Endian which use ABI v1.

Sorry, __ pc() seems to be correct. It's directly called from nmethod_entry_barrier without using a function descriptor.

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

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

Re: RFR: JDK-8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841 [v3]

Niklas Radomski
In reply to this post by Niklas Radomski
> Introduces support for _nmethod entry barriers_ and _c2i entry barriers_ on the ppc platform. Those are required to enable concurrent class unloading for compatible garbage collectors, such as Shenandoah or zGC.
>
> _This is a preparational change for the Shenandoah GC port to ppc. As such, it introduces features that the current version doesn't make use of, but that are required for the upcoming change. This way, the scope of the upcoming change is limited to Shenandoah-specific functionality; making its review a little easier._

Niklas Radomski has updated the pull request incrementally with two additional commits since the last revision:

 - Use toc offset in c2i entry barrier
 - Update comments

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2432/files
  - new: https://git.openjdk.java.net/jdk/pull/2432/files/ab354b68..202d0d35

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

  Stats: 7 lines in 3 files changed: 1 ins; 1 del; 5 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2432.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2432/head:pull/2432

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

Re: RFR: JDK-8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841 [v3]

Goetz Lindenmaier
On Mon, 8 Feb 2021 23:57:10 GMT, Niklas Radomski <[hidden email]> wrote:

>> Introduces support for _nmethod entry barriers_ and _c2i entry barriers_ on the ppc platform. Those are required to enable concurrent class unloading for compatible garbage collectors, such as Shenandoah or zGC.
>>
>> _This is a preparational change for the Shenandoah GC port to ppc. As such, it introduces features that the current version doesn't make use of, but that are required for the upcoming change. This way, the scope of the upcoming change is limited to Shenandoah-specific functionality; making its review a little easier._
>
> Niklas Radomski has updated the pull request incrementally with two additional commits since the last revision:
>
>  - Use toc offset in c2i entry barrier
>  - Update comments

Hi,
I'm not that deep in this code, but looking at other platforms this looks good.
Thanks for porting this!
Feel free to fix the comment or push as-is ... if this is possible with this tooling :)

src/hotspot/cpu/ppc/gc/shared/barrierSetAssembler_ppc.cpp line 184:

> 182:   __ bne(CCR0, skip_barrier);
> 183:
> 184:   // Class loader is weak. Determine whether the holder still alive.

'is' in comment missing:   ... whether the holder _is_ still alive.

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

Marked as reviewed by goetz (Reviewer).

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

Re: RFR: JDK-8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841 [v4]

Niklas Radomski
In reply to this post by Niklas Radomski
> Introduces support for _nmethod entry barriers_ and _c2i entry barriers_ on the ppc platform. Those are required to enable concurrent class unloading for compatible garbage collectors, such as Shenandoah or zGC.
>
> _This is a preparational change for the Shenandoah GC port to ppc. As such, it introduces features that the current version doesn't make use of, but that are required for the upcoming change. This way, the scope of the upcoming change is limited to Shenandoah-specific functionality; making its review a little easier._

Niklas Radomski has updated the pull request incrementally with one additional commit since the last revision:

  Update comment

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2432/files
  - new: https://git.openjdk.java.net/jdk/pull/2432/files/202d0d35..60f884cc

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

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

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

Re: RFR: JDK-8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841 [v3]

Niklas Radomski
In reply to this post by Goetz Lindenmaier
On Tue, 9 Feb 2021 08:28:59 GMT, Goetz Lindenmaier <[hidden email]> wrote:

>> Niklas Radomski has updated the pull request incrementally with two additional commits since the last revision:
>>
>>  - Use toc offset in c2i entry barrier
>>  - Update comments
>
> Hi,
> I'm not that deep in this code, but looking at other platforms this looks good.
> Thanks for porting this!
> Feel free to fix the comment or push as-is ... if this is possible with this tooling :)

Thank you for your reviews!

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

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

Integrated: JDK-8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841

Niklas Radomski
In reply to this post by Niklas Radomski
On Fri, 5 Feb 2021 18:04:34 GMT, Niklas Radomski <[hidden email]> wrote:

> Introduces support for _nmethod entry barriers_ and _c2i entry barriers_ on the ppc platform. Those are required to enable concurrent class unloading for compatible garbage collectors, such as Shenandoah or zGC.
>
> _This is a preparational change for the Shenandoah GC port to ppc. As such, it introduces features that the current version doesn't make use of, but that are required for the upcoming change. This way, the scope of the upcoming change is limited to Shenandoah-specific functionality; making its review a little easier._

This pull request has now been integrated.

Changeset: 906facab
Author:    Quaffel <[hidden email]>
Committer: Martin Doerr <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/906facab
Stats:     264 lines in 10 files changed: 254 ins; 0 del; 10 mod

8260372: [PPC64] Add support for JDK-8210498 and JDK-8222841

Reviewed-by: mdoerr, goetz

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

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