RFR: 8194312: Support parallel and concurrent JNI global handle processing

[Kim Barrett]

> On Jan 8, 2018, at 3:22 PM, coleen.phillimore at oracle.com wrote:
> 
> 
> 
> http://cr.openjdk.java.net/~kbarrett/8194312/open.00/src/hotspot/share/gc/shared/oopStorage.hpp.html
> 
> 463 template<typename F, typename BlockPtr> // [const] Block*
> 464 inline bool OopStorage::Block::iterate_impl(F f, BlockPtr block) {
> 
> Not my typical comment, but this (above) is too terse.

Okay.

> 489 template<typename F, typename Storage> // Storage := [const] OopStorage
> 490 inline bool OopStorage::iterate_impl(F f, Storage* storage) {
> 
> This too.  Can you add a comment before each of these that they can be instantiated with a const OopStorage or non-const.  ie that the function provides a const and non-const iteration over the block list depending on how instantiated.  For those of us who find English easier than template code.

Okay.

> 492   typedef typename Conditional<IsConst<Storage>::value, const Block*, Block*>::type BlockPtr;
> 
> add some short comment like:
> +492: // If OopStorage is const then the Block pointers are also const.

Okay.

> http://cr.openjdk.java.net/~kbarrett/8194312/open.00/src/hotspot/share/gc/shared/oopStorage.cpp.html
> 
> 109 #ifdef _WINDOWS
> 110 #pragma warning(push)
> 111 #pragma warning(disable: 4351)
> 112 #endif // _WINDOWS
> 113 OopStorage::Block::Block(const OopStorage* owner, void* memory) :
> 114   // VS2013 warns (C4351) that elements of _data will be *correctly* default
> 115   // initialized, unlike earlier versions that *incorrectly* did not do so.
> 116   // Using push/disable/pop because suppress here doesn't work.
> 
> Can you move the comment to line 110 before the pragma?

Okay.

> 134 OopStorage::Block::~Block() {
> 135 #ifdef ASSERT
> 136   // Clear fields used by block_for_ptr and entry validation, which
> 137   // might help catch bugs.
> 138   _allocated_bitmask = 0;
> 139   _owner = NULL;
> 140 #endif // ASSERT
> 141 }
> 
> Why not clear the fields anyway?  Seems like the expected behavior of the destructor.

Okay.  While doing that, it occurred to me these are dead stores,
and hence the compiler could optimize them away.  Since I'd like to
prevent that, I added some volatile casts.

And while I was at it, I added ~BlockEntry and ~BlockList with some
asserts. 

> I feel like BasicParState should be in an oopStorage.inline.hpp file to be included by the GCs that need it.   Runtime code only needs to include oopStorage.hpp and doesn't need this code.

I forgot to deal with your pre-review suggestion of moving some stuff
to oopStorage.inline.hpp.  I'd like to defer making such a change
until this review is (mostly) done, rather than clutter up diffs.

I think all (or very nearly all) of the inline definitions in
oopStorage.hpp could be moved. I think they're all related to
iteration, and I think most clients (other than GC) won't be using
iteration at all.

I think having the parallel iteration support wrapped with
#ifdef INCLUDE_ALL_GCS (as is already the case) deals with the
conditional GC usage; I think that all except Serial will eventually
use it.

The parallel iteration support could be split out into another file,
so places that only use serial iteration (like jvmti) wouldn't include
it, but I think the number of such places doesn't warrant another
file.

> 
> I don't have any other comments and looks like you've addressed most of my preview comments that I remember.  I've looked at the runtime changes for JNI and they look good.   I haven't looked at the parallel/concurrent iterator code that GC uses, and I only skimmed the test which is quite significant but good to have.
> 
> This change looks great.  Looking forward to parallel/concurrent JNI handle garbage collection and weak pointers from the runtime.

Thanks.

New webrevs (for comments from Coleen and Serguei)
full: http://cr.openjdk.java.net/~kbarrett/8194312/open.01/
incr: http://cr.openjdk.java.net/~kbarrett/8194312/open.01.inc/

Oh, and I need to go through and update all the copyrights to 2018.