RFR (XXL): JEP 243: Java-Level JVM Compiler Interface (original) (raw)

Christian Thalinger christian.thalinger at oracle.com
Fri Oct 2 21:04:48 UTC 2015

On Oct 2, 2015, at 10:15 AM, Kim Barrett <kim.barrett at oracle.com> wrote:

On Oct 1, 2015, at 7:46 PM, Christian Thalinger <christian.thalinger at oracle.com <mailto:christian.thalinger at oracle.com>> wrote:

On Sep 30, 2015, at 3:19 PM, Kim Barrett <kim.barrett at oracle.com> wrote:

Here are new webrevs against hs-comp:

http://cr.openjdk.java.net/~twisti/8136421/webrev/ <http://cr.openjdk.java.net/~twisti/8136421/webrev/> http://cr.openjdk.java.net/~twisti/8136421/hotspot/webrev/ <http://cr.openjdk.java.net/~twisti/8136421/hotspot/webrev/> I have refreshed these webrevs. They contain all the changes we discussed here plus a bunch of new tests that SQE finished. ------------------------------------------------------------------------------ src/share/vm/gc/g1/g1SATBCardTableModRefBS.cpp 260 class G1EnsureLastRefToRegion : public OopClosure { ... 270 void dooop(oop* p) { There's no way to terminate the iteration early when a match is found, so it always visits every oop in the nmethod. The early test of the result short circuits most of the work, but it would be better still if we didn't need to continue the iteration at all. This suggests there's a missing API in nmethod for iterating with early termination. This should be deferred to a followup RFE. I’m not an expert in GC-related code so please change the summary of the RFE if it’s not correct: https://bugs.openjdk.java.net/browse/JDK-8138722 Moved to gc subcomponent; we’ll take a look.

Thanks.

------------------------------------------------------------------------------ src/share/vm/gc/shared/cardTableRS.cpp Changes in CardTableRS::verifyspace function.

These don't appear to be specifically related to JVMCI. I'm not sure whether this is someone's debugging code that was left in, or if it might be viewed as an improvement. Either way, I'd prefer that it be a separate change that gets reviewed by folks interested in GC changes, rather than being buried in the JVMCI change set. Removed. Not completely. The differences below weren't removed. The old code would fail a guarantee on the first failure. The original change prints a message for each failure and then fails a guarantee at the end if there were any failures detected. That might be considered an improvement. This latest version still prints a message for each failure, but does nothing special at the end, so proceeds as if there was nothing wrong. That's not desired behavior. [Note that the removal of the unnecessary (HeapWord*) cast is in there.]

But you want me to keep that, right?

@@ -344,19 +344,26 @@ assert(jp >= begin && jp < end,_ _errmsg("Error: jp " PTRFORMAT " should be within "_ _"[begin, end) = [" PTRFORMAT "," PTRFORMAT ")",_ _p2i(jp), p2i(begin), p2i(end)));_ _oop obj = oopDesc::loaddecodeheapoop(p);_ _- guarantee(obj == NULL || (HeapWord*)obj >= boundary, - errmsg("pointer " PTRFORMAT " at " PTRFORMAT " on " + if (!(obj == NULL || (HeapWord*)obj >= boundary)) { + tty->printcr("pointer " PTRFORMAT " at " PTRFORMAT " on " "clean card crosses boundary" PTRFORMAT, - p2i((HeapWord*)obj), p2i(jp), p2i(boundary))); + p2i(obj), p2i(jp), p2i(boundary)); +#ifndef PRODUCT + obj->print(); +#endif + haderror = true; + } } public: + bool haderror; + VerifyCleanCardClosure(HeapWord* b, HeapWord* begin, HeapWord* end) : - boundary(b), begin(begin), end(end) { + boundary(b), begin(begin), end(end), haderror(false) { assert(b <= begin, errmsg("Error: boundary " PTRFORMAT " should be at or below begin " PTRFORMAT, p2i(b), p2i(begin))); assert(begin <= end, errmsg("Error: begin " PTRFORMAT " should be strictly below end " PTRFORMAT,

Sorry, it wasn’t obvious to me that you are also talking about this code. Removed:

http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/2d707abdbfba

Also, I think the cast to HeapWord* is unnecessary here: 352 p2i((HeapWord*)obj), p2i(jp), p2i(boundary)); The implicit conversion to void* by p2i should be sufficient. Removed. Thanks. ------------------------------------------------------------------------------ src/share/vm/utilities/growableArray.hpp 382 template <int compare(E&, E&)> E insertsorted(E& key) { 392 template <typename K, int compare(K&, E&)> int findsorted(K& key, bool& found) {

The parameters (except "found") and the findsorted function should be const, e.g. template <int compare(const E&, const E&)> E insertsorted(const E& key) { template <typename K, int compare(const K&, const E&)> int findsorted(const K& key, bool& found) const { Done. Thanks. ------------------------------------------------------------------------------ src/share/vm/utilities/growableArray.hpp 382 template <int compare(E&, E&)> E insertsorted(E& key) { 392 template <typename K, int compare(K&, E&)> int findsorted(K& key, bool& found) { Having the compare function be specified via a function type (e.g. a non-type template parameter) rather than an argument seems rather limiting. More general, and (IMO) easier to use, would be a type template parameter deduced from an argument, e.g. […] This can be deferred to a followup RFE. Again, please verify the summary: https://bugs.openjdk.java.net/browse/JDK-8138724 Thanks. ------------------------------------------------------------------------------ src/share/vm/utilities/growableArray.hpp 386 assert(location <= length(), "out of range");_ _387 insertbefore(location, key);_ _The assertion here is redundant with the better assertion in_ _insertbefore._ _Removed._ _Thanks._ _This should address all your comments above:_ _http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/22bab9504060 <http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/22bab9504060> Just the one remaining issue with cardTableRS.cpp discussed above.

More information about the hotspot-dev mailing list