RFR(XXS): 8227338: templateInterpreter.cpp: copy_table() needs to be safer

[Daniel D. Daugherty]

On 7/9/19 7:05 PM, Kim Barrett wrote:
>> On Jul 9, 2019, at 9:13 AM, Daniel D. Daugherty <daniel.daugherty at oracle.com> wrote:
>>
>> Hi Kim,
>>
>> Thanks for the review.
> More like drive by commentary :)

Your commentary, drive by or otherwise, is always appreciated... :-)

> I’ve never really looked at the interpreter code, and make no
> claim to understand it at all.  I *think* I understand what’s going on with this change, but I don’t
> think you should count me toward the requisite number of reviewers.

I have three (R)eviewers at the moment so no worries on that account.
Since one of your comments motivated a change to the code, I plan to
list you as a reviewer...


>
>> On 7/8/19 7:00 PM, Kim Barrett wrote:
>>>> On Jul 7, 2019, at 8:08 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>>
>>>> On 7/07/2019 6:48 pm, Erik Osterlund wrote:
>>>>> The real danger is SPARC though and its BIS instructions. I don’t have the code in front of me, but I really hope not to see that switch statement and non-volatile loop in that pd_disjoint_words_atomic() function.
>>>> sparc uses the same loop.
>>>>
>>>> Let's face it, almost no body expects the compiler to do these kinds of transformations. :(
>>> See JDK-8131330 and JDK-8142368, where we saw exactly this sort of transformation from a fill-loop
>>> to memset (which may use BIS, and indeed empirically does in some cases).  The loops in question
>>> seem trivially convertible to memcpy/memmove.
>> Very interesting reads. Thanks for pointing those out.
>>
>> src/hotspot/share/interpreter/templateInterpreter.cpp:
>>
>> DispatchTable TemplateInterpreter::_active_table;
>> DispatchTable TemplateInterpreter::_normal_table;
>> DispatchTable TemplateInterpreter::_safept_table;
>>
>> So it seems like changing _active_table to:
>>
>> volatile DispatchTable TemplateInterpreter::_active_table;
>>
>> might be a good idea... Do you concur?
> I suspect that might be a problem for various reasons.  Reading ahead, I see you’ve run into at
> least some, and deferred this to a new RFE.  So I think I’m not going to pretend to understand
> this code well enough to understand the ramifications of such a change.

Agreed. Doing this fix for Robbin (JDK-8227117) has turned into quite the
adventure... Seems to be the story of my life right now... :-)


>
>>> I’ve been reserving Atomic::load/store for cases where the location “ought” to be declared std::atomic<T> if
>>> we were using C++11 atomics (or alternatively some homebrew equivalent).  Not all places where we do
>>> stuff “atomically” is appropriate for that though (consider card tables, being arrays of bytes, where using an
>>> atomic<T> type might impose alignment constraints that are undesirable here).  I *think* just using volatile
>>> here would likely be sufficient, e.g. we should have
>>>
>>>      Copy::disjoint_words_atomic(const HeapWord* from,volatile HeapWord* to, size_t count)
>> I think this part should be taken up in the follow bug that I filed:
>>
>>      JDK-8227369 pd_disjoint_words_atomic() needs to be atomic
>>      https://bugs.openjdk.java.net/browse/JDK-8227369
> Agreed.

I pasted the above comment and the follow up comment into JDK-8227369
yesterday...

Thanks again for chiming in...

Dan


>
>> Thanks for chiming in on the review!
>>
>> Dan
>