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

[coleen.phillimore at oracle.com]

On 1/8/18 10:44 PM, Kim Barrett wrote:
>> 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 would like that it be split out into another file.  A lot of places in 
the runtime will only create and add oops to OopStorage and shouldn't 
care about iteration.  I'm fine with making this change as a further 
improvement.

Thanks,
Coleen
>
>> 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.
>