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

[Daniel D. Daugherty]

> Can you file an additional RFE to examine the uses of dispatch tables 
> for when we only have handshakes for safepoints?  And capture this 
> idea of making the tables volatile? 

Done.

     JDK-8227443 TemplateInterpreter::_active_table needs to be reexamined
     https://bugs.openjdk.java.net/browse/JDK-8227443

Feel free to update the new RFE.

Dan


On 7/9/19 10:05 AM, coleen.phillimore at oracle.com wrote:
>
>
> On 7/9/19 9:46 AM, Daniel D. Daugherty wrote:
>> On 7/9/19 9:13 AM, Daniel D. Daugherty wrote:
>>> Hi Kim,
>>>
>>> Thanks for the review.
>>>
>>>
>>> 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?
>>
>> This change would require a bunch of additional changes so I'm
>> not planning to make it (way too intrusive).
>
> Can you file an additional RFE to examine the uses of dispatch tables 
> for when we only have handshakes for safepoints?  And capture this 
> idea of making the tables volatile?
>
> thanks,
> Coleen
>>
>> Dan
>>
>>
>>>
>>>
>>>> Also see JDK-8142349.
>>>>
>>>>>> And I agree that the atomic copying API should be used when we 
>>>>>> need atomic copying. And if it turns out the implementation of 
>>>>>> that API is not atomic, it should be fixed in that atomic copying 
>>>>>> API.
>>>>> I agree to some extent, but we assume atomic load/stores of words 
>>>>> all over the place - and rightly so. The issue here is that we 
>>>>> need to hide the loop inside an API that we can somehow prevent 
>>>>> the C++ compiler from screwing up. It's hardly intuitive or 
>>>>> obvious when this is needed e.g if I simply copy three adjacent 
>>>>> words without a loop could the compiler convert that to a block 
>>>>> move that is non-atomic?
>>>>>
>>>>>> So I think this change looks good. But it looks like we are not 
>>>>>> done yet. :c
>>>>> I agree that changing the current code to use the atomic copy API 
>>>>> to convey intent is fine.
>>>> 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
>>>
>>> Thanks for chiming in on the review!
>>>
>>> Dan
>>>
>>>
>>
>