Ping: RFR: JDK-8205523: Explicit barriers for interpreter

[Erik Österlund]

Hi Roman,

On 2018-07-26 16:20, Roman Kennke wrote:
> Hi Erik,
>
> thanks for the review. I agree defaulting to READ|WRITE is even better.
>
> Updated webrevs (for other reviewers):
> Incremental:
> http://cr.openjdk.java.net/~rkennke/JDK-8205523/webrev.02.diff/
> Full:
> http://cr.openjdk.java.net/~rkennke/JDK-8205523/webrev.02/
>
> Would you agree that we could/should make this consistent across
> runtime/interpreter/c1/c2 ? It certainly would make me happier in
> runtime land, and seems required in compiler land.

Yes, agreed. I only introduced decorators for this in compilers because 
they were not needed in the interpreter and runtime. But now they are 
needed there too, having different decorators for the same thing will 
only be confusing.

Thanks,
/Erik

> Andrew: are you ok with this final patch?
>
> Roman
>
>> I gotta say I like this a lot better. With the safe defaults for the
>> non-expert, yet allowing you guys to be more precise where you know it
>> is okay, I am okay with this.
>> Just a small thing though:
>>
>> 6290 void MacroAssembler::resolve(DecoratorSet decorators, Register obj) {
>> 6291   // Use stronger ACCESS_WRITE by default.
>> 6292   if ((decorators & ACCESS_READ) == 0) {
>> 6293     decorators |= ACCESS_WRITE;
>> 6294   }
>> 6295   BarrierSetAssembler* bs =
>> BarrierSet::barrier_set()->barrier_set_assembler();
>> 6296   return bs->resolve(this, decorators, obj);
>> 6297 }
>>
>> Just in terms of semantics, I think you should really change these
>> defaults to:
>>
>> 6292   if ((decorators & (ACCESS_READ | ACCESS_WRITE)) == 0) {
>> 6293     decorators |= ACCESS_READ | ACCESS_WRITE;
>> 6294   }
>>
>> In other words, the default resolve unless specified is for ACCESS_READ
>> | ACCESS_WRITE - you may read or write primitives on the resolved oop. I
>> don't think it is necesarily intuitive that being allowed to write
>> implicitly means that you are allowed to read too; that is just an
>> implementation detail of relocating on writes as Shenandoah does things.
>> So if you just change that, you have looks good from me. And I don't
>> need another webrev for that.
>>
>> Thanks,
>> /Erik
>>
>> On 2018-07-02 13:56, Roman Kennke wrote:
>>>>> I am a fan of profile guided optimization. I would definitely not mind
>>>>> introducing these concepts in the compilers where they are with no
>>>>> doubt
>>>>> necessary (and we also have the right tools for dealing with this
>>>>> better). In fact, they already have read/write decorators that could be
>>>>> used for resolve barriers in our compilers, and can use algorithms to
>>>>> safely elide barriers where provably correct, so it makes perfect sense
>>>>> for me to use such concepts there.
>>>>> I'm just not sure that the interpreter needs to be polluted with this
>>>>> conceptual overhead, unless there is at least one benchmark that can
>>>>> show that we are solving an actual problem with this. Remember,
>>>>> premature optimizations are the root of all evil.
>>>> but efficient systems are made from thousands of tiny optimizations,
>>>> each
>>>> one too small to be measured above the noise on its own.
>>>>
>>> After some offline discussion with Erik, I want to propose a solution
>>> that 1. keeps the API simple to use, even if not sure if it's a read- or
>>> write-access (defaulting to stronger write, 2. remains flexible enough
>>> to optimize for read-only acces.
>>>
>>> This changeset introduces an API BarrierSetAssembler::resolve() which
>>> takes the 'hint' about read- vs write-access via a Decorator ACCESS_READ
>>> and ACCESS_WRITE. The API frontend in MacroAssembler::resolve() sets
>>> ACCESS_WRITE if ACCESS_READ is not explicitely set.
>>>
>>> Incremental webrev:
>>> http://cr.openjdk.java.net/~rkennke/JDK-8205523/webrev.01.diff/
>>> Full webrev:
>>> http://cr.openjdk.java.net/~rkennke/JDK-8205523/webrev.01/
>>>
>>> If this proposal gets accepted, I'd take this further (in separate RFEs):
>>> 1. make C1 use ACCESS_READ and ACCESS_WRITE instead of C1_READ_ACCESS
>>> and C1_WRITE_ACCESS. Those uses are currently no good anyway (as they
>>> are currently passed into Access API that 'knows' what it is, e.g.
>>> store_at/load_at, etc).
>>> 2. introduce same pattern for resolve() in runtime (and later, c1 and
>>> c2).
>>>
>>> What do you think?
>>>
>>> Roman
>>>
>