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

[Roman Kennke]

Hi Erik,

thank you!

This needs one more review. Andrew H: can you please take a look at it?

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/

Thanks,
Roman

> Hi Roman,
> 
> Yes. Reviewed.
> 
> Thanks,
> /Erik
> 
> On 1 Aug 2018, at 16:18, Roman Kennke <rkennke at redhat.com> wrote:
> 
>>>> 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
>>>>>>
>>>>
>>>
>>
>>