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

[Erik Osterlund]

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