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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore

This looks good!
Coleen

On 11/27/17 4:06 AM, 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

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
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
Reply | Threaded
Open this post in threaded view
|

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

Erik Österlund-2
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 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

Reply | Threaded
Open this post in threaded view
|

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

Erik Österlund-2
In reply to this post by coleen.phillimore
Hi Coleen,

Thank you for the review.

/Erik

On 2017-11-27 19:08, [hidden email] wrote:

>
> This looks good!
> Coleen
>
> On 11/27/17 4:06 AM, 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
>

Reply | Threaded
Open this post in threaded view
|

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

Thomas Schatzl
In reply to this post by coleen.phillimore
Hi,

On Mon, 2017-11-27 at 10:06 +0100, 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

  looks good.

Thomas
Reply | Threaded
Open this post in threaded view
|

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

Erik Österlund-2
Hi Thomas,

Thanks for the review.

/Erik

On 2017-11-28 14:07, Thomas Schatzl wrote:

> Hi,
>
> On Mon, 2017-11-27 at 10:06 +0100, 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
>    looks good.
>
> Thomas

Reply | Threaded
Open this post in threaded view
|

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

Per Liden
In reply to this post by coleen.phillimore
Looks good!

/Per

On 2017-11-27 10:06, 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
Reply | Threaded
Open this post in threaded view
|

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

Erik Österlund-2
Hi Per,

Thanks for the review.

/Erik

On 2017-11-28 16:07, Per Liden wrote:

> Looks good!
>
> /Per
>
> On 2017-11-27 10:06, 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

Reply | Threaded
Open this post in threaded view
|

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

David Holmes
In reply to this post by Erik Österlund-2
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 ??

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
>
Reply | Threaded
Open this post in threaded view
|

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

Erik Österlund-2
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
>>