RFR 8202201: All oop stores in the x64 interpreter are treated as volatile when using G1

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

RFR 8202201: All oop stores in the x64 interpreter are treated as volatile when using G1

coleen.phillimore
Summary: ran out of registers, generated volatile and non-volatile branches.

open webrev at http://cr.openjdk.java.net/~coleenp/8202201.01/webrev
bug link https://bugs.openjdk.java.net/browse/JDK-8202201

Tested with applications/jcstress, tier1&2 and jvmti tests (sometimes
they filled up interpreter fixed space).

putfield bytecode size is 2176 bytes now, 1248 bytes before
putstatic bytecode size is 1344 bytes now, 864 before
fast_xputfield is same 96 bytes.

Thanks,
Coleen

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8202201: All oop stores in the x64 interpreter are treated as volatile when using G1

David Holmes
Hi Coleen,

Wow. Have you run any performance tests to see what we've gained by
eliding all the unnecessary "lock addl" psuedo-mfence?

On 15/09/2018 1:44 AM, [hidden email] wrote:
> Summary: ran out of registers, generated volatile and non-volatile
> branches.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8202201.01/webrev

Generally looks good. Your webrev seems to have picked up some noise
from Mikael's unused Label changes ??

One query. Can you comment on the change to the 32-bit code where we no
longer use MO_RELAXED for the store? I'm assuming this is just a
potential optimization in the case that the store might be ordered
somehow by default. Though why this would be 32-bit specific I don't know.

Thanks,
David

> bug link https://bugs.openjdk.java.net/browse/JDK-8202201
>
> Tested with applications/jcstress, tier1&2 and jvmti tests (sometimes
> they filled up interpreter fixed space).
>
> putfield bytecode size is 2176 bytes now, 1248 bytes before
> putstatic bytecode size is 1344 bytes now, 864 before
> fast_xputfield is same 96 bytes.
>
> Thanks,
> Coleen
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR 8202201: All oop stores in the x64 interpreter are treated as volatile when using G1

coleen.phillimore


On 9/17/18 12:55 AM, David Holmes wrote:
> Hi Coleen,
>
> Wow. Have you run any performance tests to see what we've gained by
> eliding all the unnecessary "lock addl" psuedo-mfence?

No.  I would be surprised if there's any noticable difference except in
microbenchmarks.
>
> On 15/09/2018 1:44 AM, [hidden email] wrote:
>> Summary: ran out of registers, generated volatile and non-volatile
>> branches.
>>
>> open webrev at http://cr.openjdk.java.net/~coleenp/8202201.01/webrev
>
> Generally looks good. Your webrev seems to have picked up some noise
> from Mikael's unused Label changes ??

I manually made the same changes myself since I'm going to have to merge
when he checks his in.

>
> One query. Can you comment on the change to the 32-bit code where we
> no longer use MO_RELAXED for the store? I'm assuming this is just a
> potential optimization in the case that the store might be ordered
> somehow by default. Though why this would be 32-bit specific I don't
> know.

Yeah, it looks like an optimization.  Maybe Erik can tell us why he
added it.  I assume he added it.

Thanks,
Coleen

>
> Thanks,
> David
>
>> bug link https://bugs.openjdk.java.net/browse/JDK-8202201
>>
>> Tested with applications/jcstress, tier1&2 and jvmti tests (sometimes
>> they filled up interpreter fixed space).
>>
>> putfield bytecode size is 2176 bytes now, 1248 bytes before
>> putstatic bytecode size is 1344 bytes now, 864 before
>> fast_xputfield is same 96 bytes.
>>
>> Thanks,
>> Coleen
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8202201: All oop stores in the x64 interpreter are treated as volatile when using G1

Erik Österlund-2
Hi Coleen,

On 2018-09-17 14:02, [hidden email] wrote:

>
>
> On 9/17/18 12:55 AM, David Holmes wrote:
>> Hi Coleen,
>>
>> Wow. Have you run any performance tests to see what we've gained by
>> eliding all the unnecessary "lock addl" psuedo-mfence?
>
> No.  I would be surprised if there's any noticable difference except
> in microbenchmarks.

I did some Xint measurements when I accidentally found out we had this
problem, and didn't notice any difference, sadly. I was hoping for some
speedup. The whole thing made me giggle nevertheless.

>>
>> On 15/09/2018 1:44 AM, [hidden email] wrote:
>>> Summary: ran out of registers, generated volatile and non-volatile
>>> branches.
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8202201.01/webrev
>>
>> Generally looks good. Your webrev seems to have picked up some noise
>> from Mikael's unused Label changes ??
>
> I manually made the same changes myself since I'm going to have to
> merge when he checks his in.
>
>>
>> One query. Can you comment on the change to the 32-bit code where we
>> no longer use MO_RELAXED for the store? I'm assuming this is just a
>> potential optimization in the case that the store might be ordered
>> somehow by default. Though why this would be 32-bit specific I don't
>> know.
>
> Yeah, it looks like an optimization.  Maybe Erik can tell us why he
> added it.  I assume he added it.

MO_RELAXED is default, so you don't need to add it explicitly.

Also, changes look good to me. Not sure about the unused labels, but if
you say there is a plan for those, then I guess I am okay with that.

Thanks,
/Erik

> Thanks,
> Coleen
>
>>
>> Thanks,
>> David
>>
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8202201
>>>
>>> Tested with applications/jcstress, tier1&2 and jvmti tests
>>> (sometimes they filled up interpreter fixed space).
>>>
>>> putfield bytecode size is 2176 bytes now, 1248 bytes before
>>> putstatic bytecode size is 1344 bytes now, 864 before
>>> fast_xputfield is same 96 bytes.
>>>
>>> Thanks,
>>> Coleen
>>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR 8202201: All oop stores in the x64 interpreter are treated as volatile when using G1

coleen.phillimore


On 9/17/18 8:34 AM, Erik Österlund wrote:

> Hi Coleen,
>
> On 2018-09-17 14:02, [hidden email] wrote:
>>
>>
>> On 9/17/18 12:55 AM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> Wow. Have you run any performance tests to see what we've gained by
>>> eliding all the unnecessary "lock addl" psuedo-mfence?
>>
>> No.  I would be surprised if there's any noticable difference except
>> in microbenchmarks.
>
> I did some Xint measurements when I accidentally found out we had this
> problem, and didn't notice any difference, sadly. I was hoping for
> some speedup. The whole thing made me giggle nevertheless.
>
>>>
>>> On 15/09/2018 1:44 AM, [hidden email] wrote:
>>>> Summary: ran out of registers, generated volatile and non-volatile
>>>> branches.
>>>>
>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8202201.01/webrev
>>>
>>> Generally looks good. Your webrev seems to have picked up some noise
>>> from Mikael's unused Label changes ??
>>
>> I manually made the same changes myself since I'm going to have to
>> merge when he checks his in.
>>
>>>
>>> One query. Can you comment on the change to the 32-bit code where we
>>> no longer use MO_RELAXED for the store? I'm assuming this is just a
>>> potential optimization in the case that the store might be ordered
>>> somehow by default. Though why this would be 32-bit specific I don't
>>> know.
>>
>> Yeah, it looks like an optimization.  Maybe Erik can tell us why he
>> added it.  I assume he added it.
>
> MO_RELAXED is default, so you don't need to add it explicitly.
>
> Also, changes look good to me. Not sure about the unused labels, but
> if you say there is a plan for those, then I guess I am okay with that.

Thanks Erik.  I manually removed the unused labels that Mikael had so
that I could merge my changes more easily, which seems to have worked.

Coleen

>
> Thanks,
> /Erik
>
>> Thanks,
>> Coleen
>>
>>>
>>> Thanks,
>>> David
>>>
>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8202201
>>>>
>>>> Tested with applications/jcstress, tier1&2 and jvmti tests
>>>> (sometimes they filled up interpreter fixed space).
>>>>
>>>> putfield bytecode size is 2176 bytes now, 1248 bytes before
>>>> putstatic bytecode size is 1344 bytes now, 864 before
>>>> fast_xputfield is same 96 bytes.
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>
>