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

[Kim Barrett]

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 do_oop(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::verify_space 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 insert_sorted(E& key) {
>> 392   template <typename K, int compare(K&, E&)> int find_sorted(K& key, bool& found) {
>> 
>> The parameters (except "found") and the find_sorted function should be
>> const, e.g.
>> 
>> template <int compare(const E&, const E&)>
>> E insert_sorted(const E& key) {
>> 
>> template <typename K, int compare(const K&, const E&)>
>> int find_sorted(const K& key, bool& found) const {
> 
> Done.

Thanks.

>> ------------------------------------------------------------------------------ 
>> src/share/vm/utilities/growableArray.hpp   
>> 382   template <int compare(E&, E&)> E insert_sorted(E& key) {
>> 392   template <typename K, int compare(K&, E&)> int find_sorted(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       insert_before(location, key);
>> 
>> The assertion here is redundant with the better assertion in
>> insert_before.
> 
> 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.