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

[Roman Kennke]

Thanks Coleen! No problem, it's what we call 'summer hole' here ;-)

I've pushed it into submit-repo.

Thanks,
Roman


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