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

[Robbin Ehn]

Hi Kim,

On 04/10/2018 05:53 PM, 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 totally agree. But since I don't either have a good name, except for RCU, 
since this implements a very small part of a traditional RCU interface. I think 
such name might be confusing in hotspot context. Secondly when/if we change the 
underlying implementation we can easily reactor this into an interface. This 
also why I in earlier versions had the GlobalCounter on heap, so it potentially 
would be choose-able at runtime. And third I like to avoid premature abstraction 
and instead do the refactoring when need.

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

I have no strong opinion here.

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

Good suggestion fixed!

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

Fixed !

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/utilities/globalCounter.inline.hpp
> 
> Missing #include globalCounter.hpp.

Fixed!

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

Fixed!

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

I had a hard time finding a good way to do this since they also have 
dependencies to each other. I skipped this for now.

I'll post new revision to RFR mail.

Thanks, Robbin

> 
> ------------------------------------------------------------------------------
>