On 7/9/19 7:05 PM, Kim Barrett wrote:
On Jul 9, 2019, at 9:13 AM, Daniel D. Daugherty <daniel.daughe...@oracle.com>
wrote:
Hi Kim,
Thanks for the review.
More like drive by commentary :)
Your commentary, drive by or otherwise, is always appreciated... :-)
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.
I have three (R)eviewers at the moment so no worries on that account.
Since one of your comments motivated a change to the code, I plan to
list you as a reviewer...
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?
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.
Agreed. Doing this fix for Robbin (JDK-8227117) has turned into quite the
adventure... Seems to be the story of my life right now... :-)
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.
I pasted the above comment and the follow up comment into JDK-8227369
yesterday...
Thanks again for chiming in...
Dan
Thanks for chiming in on the review!
Dan
