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

[Daniel D. Daugherty]

On 7/9/19 7:17 PM, Kim Barrett wrote:
>> On Jul 9, 2019, at 9:19 AM, Daniel D. Daugherty <daniel.daugherty at oracle.com> wrote:
>> On 7/8/19 7:08 PM, Kim Barrett wrote:
>>> src/hotspot/share/interpreter/templateInterpreter.cpp
>>>   286     while (size-- > 0) *to++ = *from++;
>>>
>>> [pre-existing]
>>>
>>> This ought to be using Copy::disjoint_words.  That's even more obvious
>>> in conjunction with the change to use Copy::disjoint_words_atomic in
>>> the non-safepoint case.
>> I can make that change. Is there a specific advantage/reason that you
>> have in mind here?
> Mostly a “if we’re going to have these kinds of utility APIs because we think they
> are useful, then we really ought to use them” argument.

I like that reasoning.


> One might benefit from
> some highly tuned memcpy-like thing provided by the per-platform implementation
> of Copy::disjoint_words.

I wondered about that, but I think we're trying to get away from
such hand coding (assuming assembly here)...


> Of course, the code is sufficiently simple that a compiler
> has a reason chance of making the appropriate transformation even without us
> telling it.

:-) And the chance to make an inappropriate transformation, but we'll
deal with that if it happens (in one place)...


> The non-performance reason is a named operation is easier to read and
> understand than the corresponding explicit loop.

Also agreed. We are communicating intent by calling that function.
So I did make that change in the CR1 round...


>>> src/hotspot/share/interpreter/templateInterpreter.cpp
>>>   284   if (SafepointSynchronize::is_at_safepoint()) {
>>>
>>> I wonder how much benefit we really get from having distinct safepoint
>>> and non-safepoint cases, rather than just unconditionally using
>>> Copy::disjoint_words_atomic.
>> Sorry, I don't know the answer to that. My intention was to use
>> Copy::disjoint_words_atomic() only in the case where I knew that
>> I needed it so no potential impact on existing uses at a safepoint.
> Yeah, I just wasn’t sure how performance critical this copy is.  Hm, I see that it might
> affect the time to get out of a safepoint, so potentially getting a highly tuned
> platform-specific memcpy operation in the safepoint case might indeed be worthwhile.
> So okay.

Someone said something about a bunch of small performance improvements
add up over time... or something about death from a thousand cuts...
I can never keep those things straight... :-)

Thanks again for chiming in on the review thread.

Dan