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.hol...@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).
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
