RFR: JDK-8203157: Object equals abstraction for BarrierSetAssembler

[Roman Kennke]

Am 05.06.2018 um 17:00 schrieb Erik Österlund:
> Hi Roman,
> 
> On 2018-06-05 16:16, Roman Kennke wrote:
>> Am 04.06.2018 um 14:38 schrieb Erik Österlund:
>>> Hi Roman,
>>>
>>>   42
>>>   43   virtual void obj_equals(MacroAssembler* masm, DecoratorSet
>>> decorators,
>>>   44                           Register obj1, Register obj2);
>>>   45
>>>
>>> I don't think we need to pass in any decorators here. Perhaps one day
>>> there will be some important semantic property to deal with, but today I
>>> do not think there are any properties we care about, except possibly
>>> AS_RAW, but that would never propagate into the BarrierSetAssembler
>>> anyway.
>>>
>>> On that topic, I noticed that today we do the raw version of e.g.
>>> load_heap_oop inside of the BarrierSetAssembler, and to use it you would
>>> call load_heap_oop(AS_RAW). But the cmpoop stuff does it in a different
>>> way (cmpoop_raw in the macro assembler). I think it would be ideal if we
>>> could do it the same way, which would involve calling cmpoop with AS_RAW
>>> to get a raw oop comparison, residing in BarrierSetAssembler, with the
>>> usual hardwiring in the corresponding macro assembler function when it
>>> observes AS_RAW.
>>>
>>> So it would look something like this:
>>>
>>> void cmpoop(Register src1, Address src2, DecoratorSet decorators =
>>> AS_NORMAL);
>>>
>>> What do you think?
>> cmpoop_raw() is not the AS_RAW base implementation. It's only there to
>> help BarrierSetAssembler to implement the base
>> obj_equals(Address|Register, jobject). We cannot access cmp_literal32()
>> from outside the MacroAssembler.
> 
> In other words, there is no AS_RAW option "exposed" to public use,
> right? Maybe there is no need for raw equals in our assembly code.

Yes. That is correct.

>> The mentioned hardwiring to call straight to BSA is probably going
>> away too:
>> https://bugs.openjdk.java.net/browse/JDK-8203232
>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-May/032240.html
> 
> I'm not sure I'm convinced that is an improvement. The expected
> behaviour at the callsite is that the code in BarrierSetAssembler (which
> is the level of the hierarchy that implements raw accesses) is run, and
> nothing else. If anything else happens, it's a bug. So I hardwire that
> at the callsite to always match the expected behaviour. To instead let
> each level of the barrier class hierarchy remember to check for AS_RAW
> and delegate to the parent class in a way that ultimately has the exact
> same perceivable effect as the hardwiring, but in a much more error
> prone way, does not sound like an improvement to me. Perhaps I can be
> convinced otherwise if I understand what the concern is here and what
> problem we are trying to solve.

I don't lean very much either way. But it should be discussed under
JDK-8203232. Considering that there is no use for cmpoop_raw() except as
helper for BSA, do you agree that we don't need the AS_RAW hardwiring
for obj_equals() ? Can I consider this patch Reviewed?

Thanks, Roman