RFR(S): 8194736: Refactor weak oops in ProtectionDomain table to use the Access API

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

RFR(S): 8194736: Refactor weak oops in ProtectionDomain table to use the Access API

Erik Österlund-2
Hi,

Like other tables containing weak oop references, the ProtectionDomain
table should use the Access API.
This is a patch that does that, in a fashion very similar to what has
been done to other weak tables recently.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8194736

Webrev:
http://cr.openjdk.java.net/~eosterlund/8194736/webrev.00/

Thanks,
/Erik
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8194736: Refactor weak oops in ProtectionDomain table to use the Access API

Per Liden
Looks good!

/Per

On 2018-01-08 15:00, Erik Österlund wrote:

> Hi,
>
> Like other tables containing weak oop references, the ProtectionDomain
> table should use the Access API.
> This is a patch that does that, in a fashion very similar to what has
> been done to other weak tables recently.
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8194736
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8194736/webrev.00/
>
> Thanks,
> /Erik
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8194736: Refactor weak oops in ProtectionDomain table to use the Access API

Erik Österlund-2
Hi Per,

Thanks for the review!

/Erik

On 2018-01-08 15:36, Per Liden wrote:

> Looks good!
>
> /Per
>
> On 2018-01-08 15:00, Erik Österlund wrote:
>> Hi,
>>
>> Like other tables containing weak oop references, the ProtectionDomain
>> table should use the Access API.
>> This is a patch that does that, in a fashion very similar to what has
>> been done to other weak tables recently.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8194736
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8194736/webrev.00/
>>
>> Thanks,
>> /Erik

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8194736: Refactor weak oops in ProtectionDomain table to use the Access API

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

So this looks okay.  Does anything ever call
ProtectionDomainEntry::object() and keep it alive?

It looks like there are still literal() calls in
ProtectionDomainCacheEntry though.  Can you stomp these out first?

  void ProtectionDomainCacheEntry::verify() {
    guarantee(oopDesc::is_oop(literal()), "must be an oop");
  }


I still think this is confusing and adds too much conceptual overhead to
the runtime code,  but I plan on addressing this with WeakHandles (vm
weak oops with OopStorage) in these tables.

Thanks,
Coleen

On 1/8/18 9:00 AM, Erik Österlund wrote:

> Hi,
>
> Like other tables containing weak oop references, the ProtectionDomain
> table should use the Access API.
> This is a patch that does that, in a fashion very similar to what has
> been done to other weak tables recently.
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8194736
>
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8194736/webrev.00/
>
> Thanks,
> /Erik

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8194736: Refactor weak oops in ProtectionDomain table to use the Access API

Erik Österlund-2
Hi Coleen,

Thank you for the review.

New full webrev:
http://cr.openjdk.java.net/~eosterlund/8194736/webrev.01/

New incremental webrev:
http://cr.openjdk.java.net/~eosterlund/8194736/webrev.00_01/

On 2018-01-10 14:22, [hidden email] wrote:
>
> So this looks okay.  Does anything ever call
> ProtectionDomainEntry::object() and keep it alive?

No.

> It looks like there are still literal() calls in
> ProtectionDomainCacheEntry though.  Can you stomp these out first?

Fixed.

>
>  void ProtectionDomainCacheEntry::verify() {
>    guarantee(oopDesc::is_oop(literal()), "must be an oop");
>  }
>
>
> I still think this is confusing and adds too much conceptual overhead
> to the runtime code,  but I plan on addressing this with WeakHandles
> (vm weak oops with OopStorage) in these tables.

I am looking forward to that!

Thanks,
/Erik

>
> Thanks,
> Coleen
>
> On 1/8/18 9:00 AM, Erik Österlund wrote:
>> Hi,
>>
>> Like other tables containing weak oop references, the
>> ProtectionDomain table should use the Access API.
>> This is a patch that does that, in a fashion very similar to what has
>> been done to other weak tables recently.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8194736
>>
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8194736/webrev.00/
>>
>> Thanks,
>> /Erik
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8194736: Refactor weak oops in ProtectionDomain table to use the Access API

coleen.phillimore

Hi, Yes, the incremental change looks good.

Thanks,
Coleen

On 1/11/18 5:24 AM, Erik Österlund wrote:

> Hi Coleen,
>
> Thank you for the review.
>
> New full webrev:
> http://cr.openjdk.java.net/~eosterlund/8194736/webrev.01/
>
> New incremental webrev:
> http://cr.openjdk.java.net/~eosterlund/8194736/webrev.00_01/
>
> On 2018-01-10 14:22, [hidden email] wrote:
>>
>> So this looks okay.  Does anything ever call
>> ProtectionDomainEntry::object() and keep it alive?
>
> No.
>
>> It looks like there are still literal() calls in
>> ProtectionDomainCacheEntry though.  Can you stomp these out first?
>
> Fixed.
>
>>
>>  void ProtectionDomainCacheEntry::verify() {
>>    guarantee(oopDesc::is_oop(literal()), "must be an oop");
>>  }
>>
>>
>> I still think this is confusing and adds too much conceptual overhead
>> to the runtime code,  but I plan on addressing this with WeakHandles
>> (vm weak oops with OopStorage) in these tables.
>
> I am looking forward to that!
>
> Thanks,
> /Erik
>
>>
>> Thanks,
>> Coleen
>>
>> On 1/8/18 9:00 AM, Erik Österlund wrote:
>>> Hi,
>>>
>>> Like other tables containing weak oop references, the
>>> ProtectionDomain table should use the Access API.
>>> This is a patch that does that, in a fashion very similar to what
>>> has been done to other weak tables recently.
>>>
>>> Bug:
>>> https://bugs.openjdk.java.net/browse/JDK-8194736
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~eosterlund/8194736/webrev.00/
>>>
>>> Thanks,
>>> /Erik
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S): 8194736: Refactor weak oops in ProtectionDomain table to use the Access API

Erik Österlund-2
Hi Coleen,

Thanks for the review.

/Erik

On 2018-01-11 13:22, [hidden email] wrote:

>
> Hi, Yes, the incremental change looks good.
>
> Thanks,
> Coleen
>
> On 1/11/18 5:24 AM, Erik Österlund wrote:
>> Hi Coleen,
>>
>> Thank you for the review.
>>
>> New full webrev:
>> http://cr.openjdk.java.net/~eosterlund/8194736/webrev.01/
>>
>> New incremental webrev:
>> http://cr.openjdk.java.net/~eosterlund/8194736/webrev.00_01/
>>
>> On 2018-01-10 14:22, [hidden email] wrote:
>>>
>>> So this looks okay.  Does anything ever call
>>> ProtectionDomainEntry::object() and keep it alive?
>>
>> No.
>>
>>> It looks like there are still literal() calls in
>>> ProtectionDomainCacheEntry though.  Can you stomp these out first?
>>
>> Fixed.
>>
>>>
>>>  void ProtectionDomainCacheEntry::verify() {
>>>    guarantee(oopDesc::is_oop(literal()), "must be an oop");
>>>  }
>>>
>>>
>>> I still think this is confusing and adds too much conceptual
>>> overhead to the runtime code,  but I plan on addressing this with
>>> WeakHandles (vm weak oops with OopStorage) in these tables.
>>
>> I am looking forward to that!
>>
>> Thanks,
>> /Erik
>>
>>>
>>> Thanks,
>>> Coleen
>>>
>>> On 1/8/18 9:00 AM, Erik Österlund wrote:
>>>> Hi,
>>>>
>>>> Like other tables containing weak oop references, the
>>>> ProtectionDomain table should use the Access API.
>>>> This is a patch that does that, in a fashion very similar to what
>>>> has been done to other weak tables recently.
>>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8194736
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~eosterlund/8194736/webrev.00/
>>>>
>>>> Thanks,
>>>> /Erik
>>>
>>
>