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

[Roman Kennke]

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

I will file followup RFEs for those.

Can I consider the change reviewed by you?

Also, I need one more reviewer. Any takers, please?

Thanks!
Roman

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