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

[Andrew Dinn]

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