RFR (M) 8195099: Concurrent safe-memory-reclamation mechanism

[coleen.phillimore at oracle.com]

On 4/10/18 11:53 AM, Kim Barrett wrote:
>> On Apr 10, 2018, at 10:42 AM, Robbin Ehn <robbin.ehn at oracle.com> wrote:
>>
>> Full: http://cr.openjdk.java.net/~rehn/8195099/v2/webrev/
>> Inc : http://cr.openjdk.java.net/~rehn/8195099/v2/inc/webrev/
> ------------------------------------------------------------------------------
>
> GlobalCounter describes its implementation, but is not the least bit
> evocative of its purpose.  I don't have a good name suggestion though.

I like the name because it's simple, unique in the vm, and there isn't a 
better name that comes to mind. GlobalCounter<MutualExclusion> ?  I 
think we should keep this as is until we find a better name, which might 
take a while.

>
> ------------------------------------------------------------------------------
>
> This is being put in utilities.  Similarly we put SpinYield in
> utilities.  But I'm not sure either of those are properly placed
> there.  These are synchronization building blocks, and all other
> synchronization stuff is in runtime.

The intended use is for a concurrent hashtable in utilities that will be 
used by the StringTable (in classfile directory), so that's why 
utilities seems a good choice (esp given SpinYield there).  I think it 
would get confused with synchronizer.cpp (for Java level locks) if it 
were in runtime.

>
> ------------------------------------------------------------------------------
> src/hotspot/share/utilities/globalCounter.hpp
>
> Consider just forward declaring Thread (at namespace scope).
>
> Consider just forward declaring CounterThreadCheck at class scope,
> with the definition in the .cpp file.
>
> With those changes, don't need to #include thread.hpp in this header.
> Although globalCounter.inline.hpp still needs to #include thread.hpp,
> so maybe this doesn't matter so much?

I had this comment too.

Coleen
> ------------------------------------------------------------------------------
> src/hotspot/share/utilities/globalCounter.hpp
>
> Although CriticalSection is fully defined in this header, I think it
> can't be instantiated without including globalCounter.inline.hpp.
> Consider just forward declaring in the GlobalCounter definition, with
> the definition in the .inline.hpp.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/utilities/globalCounter.inline.hpp
>
> Missing #include globalCounter.hpp.
>
> ------------------------------------------------------------------------------
> src/hotspot/share/utilities/globalCounter.inline.hpp
>    36   OrderAccess::release_store_fence(thread->get_rcu_counter(), gbl_cnt + 1);
>
> "gbl_cnt + 1" => "gbl_cnt | COUNTER_ACTIVE".
>
> ------------------------------------------------------------------------------
> src/hotspot/share/utilities/globalCounter.hpp
>
>    74   // Must be called after finished accessing the data.
>    75   // Do not provide fence, allows load/stores moving into the critical section.
>    76   static void critical_section_end(Thread *thread);
>
> Both begin and end should have descriptions of their ordering
> behavior.  I'm thinking something similar to the descriptions we have
> for Atomic operations.
>
> ------------------------------------------------------------------------------
>