RFR: 8200557: OopStorage parallel iteration scales poorly

[Kim Barrett]

> On Apr 25, 2018, at 5:30 PM, coleen.phillimore at oracle.com wrote:
> 
> 
> http://cr.openjdk.java.net/~kbarrett/8200557/open.00/src/hotspot/share/gc/shared/oopStorage.cpp.udiff.html
> 
> +OopStorage::BasicParState::IterationData::IterationData() :
> + _segment_start(0),
> + _segment_end(0),
> + _processed(0)
> +{}
> 
> 
> Can this constructor be inlined in the declaration?  This seems out of place here (and I had to hunt for it).

I'm changing IterationData to be a simple POD-struct.  It's very
limited usage scope makes more than that overkill.

> Can some other simple accessors be inlined in the declarations as well?  That would decrease jumping around trying to understand things.
> 
> I think the OopStorage::BasicParState functions should be in a new .cpp file since there's now *src/hotspot/share/gc/shared/oopStorageParState.hpp
> *Can you do this as a follow up/cleanup RFE?

I would rather not.  The headers were split up to reduce include
dependencies.  That's not so much of an issue for oopStorage.cpp.  And
the implementation of OopStorage::BasicParState is pretty intimately
tied to the rest of OopStorage.


> http://cr.openjdk.java.net/~kbarrett/8200557/open.00/src/hotspot/share/gc/shared/oopStorage.hpp.udiff.html
> 
>   class Block;                  // Forward decl; defined in .inline.hpp file.
> + class BlockArray; // Forward decl; defined in .inline.hpp file.
>   class BlockList;              // Forward decl for BlockEntry friend decl.
> 
> Can you give a short description of these, ie:
> 
>   class Block;                  // Block of oop* stored in OopStorage
> + class BlockArray; // Storage for all active oop Blocks
>   class BlockList;              // List of oop Blocks from which are available for allocation
> 
> Or maybe a short description over the class declarations?  The trouble with these generic names is you forget why they exist.

I've added some/more descriptions, and moved some to hopefully be
better placed to be found.


> I'd call BlockList a AllocationBlockList and BlockArray an ActiveBlockArray

For naming, I think BlockArray => ActiveArray, BlockList =>
AllocateList, BlockEntry => AllocateListEntry.  Recall that before
this change there wasn't an active *array*. rather there were two
BlockLists, active and allocate, so the generic name made more sense
then.  I'll do this as a followup RFE; there's also a simplification
to be made to BlockList, now that there's only one, which I was
planning to do as a followup.

> http://cr.openjdk.java.net/~kbarrett/8200557/open.00/src/hotspot/share/gc/shared/oopStorage.inline.hpp.frames.html
> 
> 38 class OopStorage::BlockArray {
> 
> 
> This would be a good place to comment the purpose of this and a line about refcounting.
> 
> http://cr.openjdk.java.net/~kbarrett/8200557/open.00/src/hotspot/share/gc/shared/oopStorage.hpp.frames.html
> 
> Why does BlockList need to be included in the .hpp file?

BlockList needs to be in the .hpp file because there is a member in
OopStorage, requiring it to be complete. Block and Block can be
forward declared. But I now realize that BlockEntry could also be
forward-declared and moved to the .inline.hpp file.

> http://cr.openjdk.java.net/~kbarrett/8200557/open.00/src/hotspot/share/gc/shared/oopStorage.cpp.frames.html
> 
> All these places have block_count() which are indistinguishable. It looks like _block_count for the BlockArray should always be acquired outside of BlockArray class or rather give that as the only public option to prevent mistakes.  It seems like there wouldn't be much performance cost to calling load_acquire once at the safepoint iteration.

block_count() (and _block_count) are used in places where we know
we're in a safepoint or that we hold the _allocate_mutex.
block_count_acquire() is used where neither of those is true.

Responding to Erik's questions about block_count() vs
block_count_acquire(), OopStorage::block_count() and
OopStorage::total_memory_usage() are in the latter case. I've waffled
over whether it is better to use an unnecessary acquire or possibly
need to explain why it isn't necessary.  Of course, then one may need
to explain an apparently unnecessary acquire.

After looking over things in this area yet again, I think there are
some places where it's better to directly use the _block_count member
rather than an accessor function anyway, so I'm doing that.  And an
unnecessary acquire seems like it needs as much explaination as not
having it, so removing those too.

> Comment about _why_ refcounts and what do they do here would be nice.
> 
> 151 void OopStorage::BlockArray::increment_refcount() const {
> 
> 
> 567 void OopStorage::relinquish_block_array(BlockArray* array) const {
> 568 if (array->decrement_refcount()) {
> 569 assert(array != _active_array, "invariant");
> 570 BlockArray::destroy(array);
> 571 }
> 572 }
> 
> 
> Here array is a pointer to the active array so iterators could destroy the array if they are the last ones to use it.  Is this right?

Yes. 

> I've read through most of the rest of the code and it looks good apart from how complicated it is, which I don't know how to suggest to improve it other than some reminder comments and maybe more descriptive names.

New webrevs:
full: http://cr.openjdk.java.net/~kbarrett/8200557/open.00/
incr: http://cr.openjdk.java.net/~kbarrett/8200557/open.01.inc/