RFR (S): 8191888: Refactor ClassLoaderData::remove_handle to use the Access API

[Erik Österlund]

Hi David,

On 2017-11-28 22:28, David Holmes wrote:
> Hi Erik,
>
> On 28/11/2017 10:19 PM, Erik Österlund wrote:
>> Hi David,
>>
>> That is a good question.
>> There is a description (written by a GC person, but still) for each 
>> decorator in the access.hpp file where the decorators are declared. I 
>> try to go in to some details there when you should and should not use 
>> a decorator. If we find that the text might have been biased from a 
>> GC perspective, we can update the comments.
>
> The comments serve as a reference, but I think we may lack a 
> "tutorial". :) As I think someone commented earlier re the Access API, 
> how do I know that, for example, IN_CONCURRENT_ROOT applies when I 
> have no idea what may or may not be scanned during safepoint ??

Would it be helpful if I wrote a little tutorial in a comment section in 
access.hpp? Hopefully, as the code gets more populated with calls to 
this thing, it starts to look more familiar as well.

As for whether to use IN_CONCURRENT_ROOT or not, the basic idea is that 
if you do not know whether your root may be processed outside of 
safepoints, then use IN_CONCURRENT_ROOT conservatively. False positives 
are fine and only come with a small performance loss for some collectors 
(e.g. cause unnecessary G1 pre-write SATB barrier). False negatives on 
the other hand can be fatal (e.g. not performing necessary G1 pre-write 
SATB barrier). It is a little bit like using MO_SEQ_CST vs MO_ACQUIRE. 
Using MO_SEQ_CST when you are not sure is strictly safer but comes with 
a performance penalty. If you for example build thread-local values in 
value types, you might not want to take that performance penalty when 
you know your values are conceptually part of your stack and are 
processed in safepoints.

As we move forward, we might change whether assuming roots may be 
processed concurrently, outside of safepoints, should be default or not. 
AFAIK, it is currently the rare exception rather than the rule. But 
perhaps things will change over time.

Thanks,
/Erik

> But that's OT.
>
> Thanks,
> David
>
>
>
>> The previous situation has been to use naked accesses, and then 
>> suddenly a GC person jumps in moaning about the lack of explicit #if 
>> INCLUDE_ALL_GCS if (UseG1) SATB barrier goo, or manual insertion of 
>> post-write barriers if a CAS was successful. There was no good way of 
>> knowing you had to manually insert these barriers, what barriers were 
>> offered to shared code, or what their semantics were.
>>
>> Now at least you have a set of decorators to choose from, with some 
>> automatic verification if you select nonsense decorators, and 
>> documentation in one place what they are used for and what they mean. 
>> So if you do not know what you need, at least you can read the 
>> descriptions and hopefully figure it out, which was impossible before.
>>
>> Having said that, I believe things like Coleen's OopHandle and 
>> PhantomOopHandle will build a layer on top of Access with possibly a 
>> few less details exposed that are even simpler to use for the 
>> conservative non-GC person that just wants the thing to work as 
>> simple as possible and wants to stay as far away from GC land as 
>> possible, which is sane.
>>
>> And if you feel uncertain, you can always loop me in any time, and I 
>> will probably say "there's a decorator for that".
>>
>> I hope this answers your question.
>>
>> Thanks,
>> /Erik
>>
>> On 2017-11-28 07:59, David Holmes wrote:
>>> Hi Erik,
>>>
>>> Is there a non-GC person's guide to what the different forms of 
>>> RootAccess<XXX> mean and when to use them? Or do we expect a GC 
>>> person to always jump in to show where such things are needed? ;-)
>>>
>>> Thanks,
>>> David
>>>
>>> On 27/11/2017 7:06 PM, Erik Österlund wrote:
>>>> Hi,
>>>>
>>>> The ClassLoaderData::remove_handle() member function currently uses 
>>>> explicit G1 SATB barriers to remove an oop from the root set, as 
>>>> these handles are not necessarily walked by the GC in a safepoint. 
>>>> Therefore G1 needs pre-write barriers.
>>>>
>>>> This should now be modeled as a 
>>>> RootAccess<IN_CONCURRENT_ROOT>::oop_store instead. This maps to 
>>>> performing a pre-write SATB barrier with G1, but other GCs are free 
>>>> to do other things as necessary.
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8191888/webrev.00/
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8191888
>>>>
>>>> Thanks,
>>>> /Erik
>>