> On Jul 9, 2019, at 9:13 AM, Daniel D. Daugherty <[email protected]> > wrote: > > Hi Kim, > > Thanks for the review.
More like drive by commentary :) 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. > On 7/8/19 7:00 PM, Kim Barrett wrote: >>> On Jul 7, 2019, at 8:08 PM, David Holmes <[email protected]> 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. >> 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. > > Thanks for chiming in on the review! > > Dan
