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

Wed Aug 8 16:03:37 UTC 2018

I've looked at this and I think it looks good to me.  Sorry for the 
delay in reviewing it.

Coleen

On 8/8/18 5:40 AM, Roman Kennke wrote:
> 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
>>>>>>>
>>>