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

[Christian Thalinger]

> 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 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.

Thanks.

> 
>>> ------------------------------------------------------------------------------
>>> 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.]

But you want me to keep that, right?

> 
> @@ -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,

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 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 <http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/22bab9504060>
> 
> Just the one remaining issue with cardTableRS.cpp discussed above.