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

[Christian Thalinger]

> 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

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

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

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

> 
> ------------------------------------------------------------------------------ 
> 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.
> 
> template<typename Compare>
> E insert_sorted(const E& key, Compare compare) {
>  ...
>  int location = find_sorted(key, found, compare);
>  ...
> }
> 
> template<typename K, typename Compare>
> int find_sorted(const K& key, bool& found, Compare compare) const {
>  ...
> }
> 
> Let
> - ga is a GrowableArray<T>
> - my_value is an instance f T
> - my_compare is a function of type int (*)(const T&, const T&)
> 
> Old usage:
> 
>  ga.insert_sorted<my_compare>(my_value);
>  ga.find_sorted<T, my_compare>(my_value, found);
> 
> New usage:
> 
>  ga.insert_sorted(my_value, my_compare);
>  ga.find_sorted(my_value, found, my_compare);
> 
> and new usage also allows the use of function objects, not just
> pointers to functions.
> 
> This can be deferred to a followup RFE.  

Again, please verify the summary:

https://bugs.openjdk.java.net/browse/JDK-8138724

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

> 
> ------------------------------------------------------------------------------
> 

This should address all your comments above:

http://hg.openjdk.java.net/graal/graal-jvmci-9/hotspot/rev/22bab9504060