This is a fairly huge backport from Shenandoah's dev line (then jdk9) to shenandoah/jdk8u and from there to aarch64-port/jdk8u-shenandoah. It brings: - bunch of new features (e.g. a separate update-refs phase, parallel and concurrent code cache scanning, etc) - much improved performance (both macro- and micro-performance) - much improved stability (we did test this fairly extensively, and added many regression tests) - much improved usability (e.g. better option handling, much more useful output) - many bugfixes and tests and code improvements - lesser diff compared to upstream All of this has baked considerably in shenandoah/jdk8u for a while, gone through testing and testing and testing (also, testing by interested early adopters) and fixing. I simply transplanted all relevant changesets from shenandoah/jdk8u to aarch64-port/jdk8u-shenandoah with no fuzz or any problems. It's then been tested again by running the hotspot_gc_shenandoah jtreg test group (i.e. the whole Shenandoah jtreg test suite), again without issues. A list of all changesets can be found here: http://cr.openjdk.java.net/~rkennke/aarch64-import-2017-07-24/out.txt <http://cr.openjdk.java.net/%7Erkennke/aarch64-import-2017-07-24/out.txt> The full webrev: http://cr.openjdk.java.net/~rkennke/aarch64-import-2017-07-24/webrev.00/ <http://cr.openjdk.java.net/%7Erkennke/aarch64-import-2017-07-24/webrev.00/> Ok to push?
On 24/07/17 16:12, Roman Kennke wrote:
Ok to push? This is way too big to check absolutely thoroughly so in reality it is going to have to go through on the nod. That's probably ok given the amount of testing you have done. However, I did look through the mega-change set to see if there were any things that looked obviously dubious or questionable or that might still run correctly but be subtly wrong.
I didn't find anything I could swear was incorrect, even with stuff like memory-sync operations -- some of which I was puzzled by and then convinced myself were ok. I spotted a few tiny things I thought were worth asking about -- not that they are necessarily wrong but just that I was not clear that they are right: rev 9575 : [backport] implicit null checks broken on aarch64 src/share/vm/asm/assembler.cpp Can you explain the need to use the 48 bit mask? Is it to do with the subtract of adj making the sign of the result go negative? If so (or even if not :-) then instead of subtracting adj form the offset why not simply add adj to base (suitably scaled) before doing the compare? I ask because it looks to me like doing this mask operation will mean that we would go on to do an explicit null check when the top 16 bits are non-zero rubbish and the bottom bits happen to be in range. That's not critical because the rubbish bits will get caught at the next check but I just wondered why the mask was needed. rev 9597 : Revert G1 changes and bring shared BitMap src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp In method clean_nmethod(nmethod* nm) the comment says + // Mark that this thread has been cleaned/unloaded. + // After this call, it will be safe to ask if this nmethod was unloaded or not. + nm->set_unloading_clock(nmethod::global_unloading_clock()); "thread has been cleaned/unloaded"? Should that not be nmethod? rev ??? [not clear which one it was] src/share/vm/gc_implementation/shenandoah/shenandoahHeapRegion.hpp + // Convert to jint with sanity checking + inline static size_t region_size_words_jint() { + assert (ShenandoahHeapRegion::RegionSizeWords <= (size_t)max_jint, "sanity"); + return (jint)ShenandoahHeapRegion::RegionSizeWords; + } Why is the return type a size_t rather than a jint? So, (possibly) modulo responses to these 3 questions, ship it! regards, Andrew Dinn -----------
Hi Andrew, thanks for looking through this!
rev 9575 : [backport] implicit null checks broken on aarch64 src/share/vm/asm/assembler.cpp
Can you explain the need to use the 48 bit mask? Is it to do with the subtract of adj making the sign of the result go negative? If so (or even if not :-) then instead of subtracting adj form the offset why not simply add adj to base (suitably scaled) before doing the compare?
I ask because it looks to me like doing this mask operation will mean that we would go on to do an explicit null check when the top 16 bits are non-zero rubbish and the bottom bits happen to be in range. That's not critical because the rubbish bits will get caught at the next check but I just wondered why the mask was needed. It is explained here: http://mail.openjdk.java.net/pipermail/shenandoah-dev/2017-June/002756.html
rev 9597 : Revert G1 changes and bring shared BitMap src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
In method clean_nmethod(nmethod* nm) the comment says
+ // Mark that this thread has been cleaned/unloaded. + // After this call, it will be safe to ask if this nmethod was unloaded or not. + nm->set_unloading_clock(nmethod::global_unloading_clock());
"thread has been cleaned/unloaded"? Should that not be nmethod? Probably, but this is only a comment, and not even ours. We only re-instanted the code to where it was before (it's G1 code), so this is one minimize-diff-to-upstream change :-) rev ??? [not clear which one it was] src/share/vm/gc_implementation/shenandoah/shenandoahHeapRegion.hpp
+ // Convert to jint with sanity checking + inline static size_t region_size_words_jint() { + assert (ShenandoahHeapRegion::RegionSizeWords <= (size_t)max_jint, "sanity"); + return (jint)ShenandoahHeapRegion::RegionSizeWords; + }
Why is the return type a size_t rather than a jint?
Good catch! This looks like a genuine mistake! I'll push a fix to shenandoah-jdk10 and let it trickle down to jdk9, jdk8 and from there into this. Do you think it's ok to push with the size_t changed to jint? diff --git a/src/share/vm/gc/shenandoah/shenandoahHeapRegion.hpp b/src/share/vm/gc/shenandoah/shenandoahHeapRegion.hpp --- a/src/share/vm/gc/shenandoah/shenandoahHeapRegion.hpp +++ b/src/share/vm/gc/shenandoah/shenandoahHeapRegion.hpp @@ -81,7 +81,7 @@ } // Convert to jint with sanity checking - inline static size_t region_size_words_jint() { + inline static jint region_size_words_jint() { assert (ShenandoahHeapRegion::RegionSizeWords <= (size_t)max_jint, "sanity"); return (jint)ShenandoahHeapRegion::RegionSizeWords; }
On 25/07/17 19:28, Roman Kennke wrote:
Can you explain the need to use the 48 bit mask? It is explained here: http://mail.openjdk.java.net/pipermail/shenandoah-dev/2017-June/002756.html
Ah, I see. When needs_explicit_null_check is called from the signal handler this value is coming straight out of info->si_addr (where siginfo_t* info). Hmm, it turns out that this is complicated. I think it's ok for now to push the code as is with one minor change to the comment: - // AArch64 uses 48-bit addresses + // AArch64 addresses passed from the signal handler may have + // their top 8 bits zeroed. That affects the case where + // Shenandoah tries to load a Brooks pointer via a null oop. I have identified why this truncation is happening and what it means. I will start a follow-up thread to discuss why this needs i) a better explanation in the code ii) possible rework "thread has been cleaned/unloaded"? Should that not be nmethod?
Probably, but this is only a comment, and not even ours. We only re-instanted the code to where it was before (it's G1 code), so this is one minimize-diff-to-upstream change :-)
Ok, I guess we can leave this for later generations of devs to go "Tchh, ha!" over.
Why is the return type a size_t rather than a jint?
Good catch! This looks like a genuine mistake! I'll push a fix to shenandoah-jdk10 and let it trickle down to jdk9, jdk8 and from there into this.
Hurrah, so it /was/ worth reviewing after all!
Do you think it's ok to push with the size_t changed to jint? Sure. It was arguably 'ok' to push even without my suggested changes but I had to justify my time spent checking it somehow :-)
It was very impressive to see how many improvements you and Aleksey managed to add. Nice job! regards, Andrew Dinn -----------
On 24 July 2017 at 16:12, Roman Kennke <rkennke@redhat.com> wrote:
This is a fairly huge backport from Shenandoah's dev line (then jdk9) to shenandoah/jdk8u and from there to aarch64-port/jdk8u-shenandoah. It brings:
- bunch of new features (e.g. a separate update-refs phase, parallel and concurrent code cache scanning, etc)
- much improved performance (both macro- and micro-performance)
- much improved stability (we did test this fairly extensively, and added many regression tests)
- much improved usability (e.g. better option handling, much more useful output)
- many bugfixes and tests and code improvements
- lesser diff compared to upstream
All of this has baked considerably in shenandoah/jdk8u for a while, gone through testing and testing and testing (also, testing by interested early adopters) and fixing. I simply transplanted all relevant changesets from shenandoah/jdk8u to aarch64-port/jdk8u-shenandoah with no fuzz or any problems.
It's then been tested again by running the hotspot_gc_shenandoah jtreg test group (i.e. the whole Shenandoah jtreg test suite), again without issues.
A list of all changesets can be found here:
http://cr.openjdk.java.net/~rkennke/aarch64-import-2017-07-24/out.txt <http://cr.openjdk.java.net/%7Erkennke/aarch64-import-2017-07-24/out.txt>
The full webrev:
http://cr.openjdk.java.net/~rkennke/aarch64-import-2017-07-24/webrev.00/ <http://cr.openjdk.java.net/%7Erkennke/aarch64-import-2017-07-24/webrev.00/>
Ok to push?
The update breaks building without precompiled headers. See attached patch. Thanks, -- Andrew :) Senior Free Java Software Engineer Red Hat, Inc. (http://www.redhat.com) Web Site: http://fuseyism.com Twitter: https://twitter.com/gnu_andrew_java PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 579D D154 0ED2 3E04 C5A0 CFDA 0F9B 3596 4222
Am 27.07.2017 um 08:02 schrieb Andrew Hughes:
On 24 July 2017 at 16:12, Roman Kennke <rkennke@redhat.com> wrote:
This is a fairly huge backport from Shenandoah's dev line (then jdk9) to shenandoah/jdk8u and from there to aarch64-port/jdk8u-shenandoah. It brings:
- bunch of new features (e.g. a separate update-refs phase, parallel and concurrent code cache scanning, etc)
- much improved performance (both macro- and micro-performance)
- much improved stability (we did test this fairly extensively, and added many regression tests)
- much improved usability (e.g. better option handling, much more useful output)
- many bugfixes and tests and code improvements
- lesser diff compared to upstream
All of this has baked considerably in shenandoah/jdk8u for a while, gone through testing and testing and testing (also, testing by interested early adopters) and fixing. I simply transplanted all relevant changesets from shenandoah/jdk8u to aarch64-port/jdk8u-shenandoah with no fuzz or any problems.
It's then been tested again by running the hotspot_gc_shenandoah jtreg test group (i.e. the whole Shenandoah jtreg test suite), again without issues.
A list of all changesets can be found here:
http://cr.openjdk.java.net/~rkennke/aarch64-import-2017-07-24/out.txt <http://cr.openjdk.java.net/%7Erkennke/aarch64-import-2017-07-24/out.txt>
The full webrev:
http://cr.openjdk.java.net/~rkennke/aarch64-import-2017-07-24/webrev.00/ <http://cr.openjdk.java.net/%7Erkennke/aarch64-import-2017-07-24/webrev.00/>
Ok to push?
The update breaks building without precompiled headers. See attached patch.
Thanks for the heads-up and patch. I pushed the fix to all Shenandoah trees and aarch64-port/jdk8u-shenandoah and added a new tag: aarch64-shenandoah-jdk8u141-b16-shenandoah-merge-2017-07-27 I'll also see to it that we (the Shenandoah team) set up a regular build without precompiled headers. This seems to keep biting us... Thanks, Roman
participants (3)
-
Andrew Dinn
-
Andrew Hughes
-
Roman Kennke