On 7/9/19 7:17 PM, Kim Barrett wrote:
On Jul 9, 2019, at 9:19 AM, Daniel D. Daugherty <daniel.daughe...@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
