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