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

[Roman Kennke]

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.

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
>>
>