[aarch64-port-dev ] RFR: Backport Shenandoah since last merge

Roman Kennke rkennke at redhat.com
Tue Jul 25 18:28:42 UTC 2017


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;
   }



More information about the aarch64-port-dev mailing list