RFR: 8200557: OopStorage parallel iteration scales poorly

Wed May 2 21:42:52 UTC 2018

Changes look good.

On 5/1/18 5:13 PM, Kim Barrett wrote:
>> 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.

http://cr.openjdk.java.net/~kbarrett/8200557/open.01.inc/src/hotspot/share/gc/shared/oopStorageParState.inline.hpp.udiff.html

Just because it's a POD, you could still have the interesting function?

- void note_processed_segment() { _processed += _segment_end - 
_segment_start; }

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

All right.
>
>> 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 like the new comments.  I still think pointing out that these are 
forward declarations and not what they are used for here is not very 
interesting, though.  A short 1/2 liner as suggested would be better in 
case one doesn't need to read the rest of the code.
>
>> 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.

That would be fine.  Hopefully an easier code review :)
>
>> 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.

Okay this is less confusing about when you need acquire vs not. It's 
volatile so that's good.
>> 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/
>
>
Thanks for moving the comments into the implementation.  I like where 
they are now.

If you decide to update any more comments, I don't need to see another 
webrev.

thanks,
Coleen