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

[Kim Barrett]

> 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 :)  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.

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

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

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