RFR (XXL): JEP 243: Java-Level JVM Compiler Interface (original) (raw)
Kim Barrett kim.barrett at oracle.com
Fri Oct 2 20:15:26 UTC 2015
- Previous message: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface
- Next message: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
On Oct 1, 2015, at 7:46 PM, Christian Thalinger <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.
------------------------------------------------------------------------------ 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.]
@@ -344,19 +344,26 @@ assert(jp >= _begin && jp < _end, err_msg("Error: jp " PTR_FORMAT " should be within " "[_begin, _end) = [" PTR_FORMAT "," PTR_FORMAT ")", p2i(jp), p2i(_begin), p2i(_end))); oop obj = oopDesc::load_decode_heap_oop(p);
- guarantee(obj == NULL || (HeapWord*)obj >= _boundary,
err_msg("pointer " PTR_FORMAT " at " PTR_FORMAT " on "
- if (!(obj == NULL || (HeapWord*)obj >= _boundary)) {
tty->print_cr("pointer " PTR_FORMAT " at " PTR_FORMAT " on " "clean card crosses boundary" PTR_FORMAT,
p2i((HeapWord*)obj), p2i(jp), p2i(_boundary)));
p2i(obj), p2i(jp), p2i(_boundary));
+#ifndef PRODUCT
obj->print();
+#endif
had_error = true;- }
}
public:
- bool had_error;
- VerifyCleanCardClosure(HeapWord* b, HeapWord* begin, HeapWord* end) :
- _boundary(b), _begin(begin), _end(end) {
- _boundary(b), _begin(begin), _end(end), had_error(false) { assert(b <= begin, err_msg("Error: boundary " PTR_FORMAT " should be at or below begin " PTR_FORMAT, p2i(b), p2i(begin))); assert(begin <= end, err_msg("Error: begin " PTR_FORMAT " should be strictly below end " PTR_FORMAT,
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
Just the one remaining issue with cardTableRS.cpp discussed above.
- Previous message: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface
- Next message: RFR (XXL): JEP 243: Java-Level JVM Compiler Interface
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]