[16] RFR: 8248391: Unify handling of all OopStorage instances in weak root processing

[Kim Barrett]

> On Jul 16, 2020, at 7:05 AM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:

Regarding some things that you would have preferred as separate changes, see below.

> On 07.07.20 15:47, Kim Barrett wrote:
>> I think the proposed terminology for the GC -> OopStorage owner callback was
>> too vague and generic; "notify" and "notification" are relatively
>> content-free names.  So I've renamed the functions and types.  Also, I think
>> "can_notify" should be "should_notify" (and then adjusted for the generic
>> "notify").
>> I've also changed (and documented) the semantics; […]
> 
> I would have somewhat like this change to be separate if possible, but okay.

Where I said “I’ve also changed” I meant with respect to Erik’s original proposal.
What I have proposed reverts Erik’s change in this area back to the current behavior.

>> The (anonymous) enum of OopStorageSet iterator indices no longer serves any
>> purpose; I've changed them to be ordinary in-class constants.  With the
>> original code the enum was used to automatically assign values to some of
>> the enumerators, but with the removal of named indices for each storage,
>> there was only one autmatically assigned enumerator.

It seemed strange to me to delete the reasons for using an enum but still use an enum.

>> I've changed WeakProcessor::Task to also use the new
>> OopStorageSetWeakParState.  This involved adding support for client access
>> to the states, similar to what's in the strong parstate.  (I'm not sure the
>> weak parstate should provide an oops_do that counts and reports the dead,
>> but I've left that for now.)

It seemed strange to me to introduce OopStorageSetWeakParState as part of this change,
but only use it in some places (ZGC and Shenandoah) but not others (WeakProcessor).

>> Fixed a "bug" that WeakProcessor::oops_do(OopClosure*) counted and reported
>> dead entries, even though it never clears entries.  It is used for
>> iterations that shouldn't be reporting dead, such as for the adjust pointers
>> phase of mark-sweep-compact.

Yeah, that could have been done separately.

>> Besides adapting to the above changes, I moved StringTable's clearing of the
>> _has_work flag to the end of its do_concurrent_work.  This avoids queuing
>> a new cleanup request in the middle of a concurrent cleanup that might
>> eliminate the need for further cleaning.

That's really part of reverting Erik’s change to the reporting behavior.  I suspected that
Erik’s reporting change was in part a result of the previous placement of that clearing;
Erik’s change reduces the likelihood of excess cleaning requests with the old placement.

> Thanks! Looks good apart from one pre-existing typo: in oopStorage.hpp:164 there is "if any..", ie. one full stop too many.

Fixed.

Thanks for reviewing.