RFR (L) 8174749: Use hash table/oops for MemberName table

[Stefan Karlsson]

Hi Coleen,

I'm mainly reviewing the GC specific parts.

http://cr.openjdk.java.net/~coleenp/8174749.01/webrev/src/share/vm/prims/resolvedMethodTable.cpp.html

  143 void ResolvedMethodTable::unlink_or_oops_do(BoolObjectClosure* 
is_alive, OopClosure* f) {
...
  151       if (f != NULL) {
  152         f->do_oop((oop*)entry->literal_addr());
  153         p = entry->next_addr();
  154       } else {
  155         if (!is_alive->do_object_b(entry->literal())) {
  156           _oops_removed++;
  157           if (log_is_enabled(Debug, membername, table)) {
  158             ResourceMark rm;
  159             Method* m = 
(Method*)java_lang_invoke_ResolvedMethodName::vmtarget(entry->literal());
  160             log_debug(membername, table) ("ResolvedMethod vmtarget 
entry removed for %s index %d",
  161 
m->name_and_sig_as_C_string(), i);
  162           }
  163           *p = entry->next();
  164           _the_table->free_entry(entry);
  165         } else {
  166           p = entry->next_addr();
  167         }
  168       }

This code looks backwards to me. If you pass in both an is_alive closure 
and an f (OopClosure), then you ignore the is_alive closure. This will 
break if we someday want to clear these entries during a copying GC. 
Those GCs want to unlink dead entries and apply the f closure to the 
oop*s of the live entries.

Could you change this to mimic the code in the StringTable?:

http://hg.openjdk.java.net/jdk10/hs/hotspot/file/094298f42cc7/src/share/vm/classfile/stringTable.cpp

   if (is_alive->do_object_b(entry->literal())) {
     if (f != NULL) {
       f->do_oop((oop*)entry->literal_addr());
     }
     p = entry->next_addr();
   } else {
     *p = entry->next();
     the_table()->free_entry(entry);
     (*removed)++;
   }

------------------------------------------------------------------------------

http://cr.openjdk.java.net/~coleenp/8174749.01/webrev/src/share/vm/gc/g1/g1CollectedHeap.cpp.frames.html

3888   // The parallel work done by all worker threads.
3889   void work(uint worker_id) {
3890     // Do first pass of code cache cleaning.
3891     _code_cache_task.work_first_pass(worker_id);
3892
3893     // Let the threads mark that the first pass is done.
3894     _code_cache_task.barrier_mark(worker_id);
3895
3896     // Clean the Strings and Symbols.
3897     _string_symbol_task.work(worker_id);
3898
3899     // Wait for all workers to finish the first code cache cleaning 
pass.
3900     _code_cache_task.barrier_wait(worker_id);
3901
3902     // Do the second code cache cleaning work, which realize on
3903     // the liveness information gathered during the first pass.
3904     _code_cache_task.work_second_pass(worker_id);
3905
3906     // Clean all klasses that were not unloaded.
3907     _klass_cleaning_task.work();
3908
3909     // Clean unreferenced things in the ResolvedMethodTable
3910     _resolved_method_cleaning_task.work();
3911   }

The GC workers wait in the barrier_wait function as long as there are 
workers left that have not passed the barrier_mark point. If you move 
the _resolved_method_cleaning_task.work() to somewhere between 
barrier_mark and barrier_wait, there might be some opportunity for one 
of the workers to do work instead of waiting in the mark_wait barrier.

------------------------------------------------------------------------------

3876   G1MemberNameCleaningTask      _resolved_method_cleaning_task;

There seems to be a naming confusion in this patch. Sometimes it talks 
about MemberNames and sometimes ResolvedMethods. Could you make this 
more consistent throughout the patch?

------------------------------------------------------------------------------

http://cr.openjdk.java.net/~coleenp/8174749.01/webrev/src/share/vm/prims/resolvedMethodTable.cpp.html

   25 #include "precompiled.hpp"
   26 #include "gc/shared/gcLocker.hpp"
   27 #include "memory/allocation.hpp"
   28 #include "oops/oop.inline.hpp"
   29 #include "oops/method.hpp"
   30 #include "oops/symbol.hpp"
   31 #include "prims/resolvedMethodTable.hpp"
   32 #include "runtime/handles.inline.hpp"
   33 #include "runtime/mutexLocker.hpp"
   34 #include "utilities/hashtable.inline.hpp"
   35 #include "utilities/macros.hpp"
   36 #if INCLUDE_ALL_GCS
   37 #include "gc/g1/g1CollectedHeap.hpp"
   38 #include "gc/g1/g1SATBCardTableModRefBS.hpp"
   39 #include "gc/g1/g1StringDedup.hpp"
   40 #endif

I don't thing you should include gcLocker.hpp, g1CollectedHeap.hpp, or 
g1StringDedup.hpp from this file.

Thanks,
StefanK

On 2017-05-17 18:01, coleen.phillimore at oracle.com wrote:
> Summary: Add a Java type called ResolvedMethodName which is immutable
> and can be stored in a hashtable, that is weakly collected by gc
>
> Thanks to John for his help with MemberName, and to-be-filed RFEs for
> further improvements.  Thanks to Stefan for GC help.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8174749.01/webrev
> open webrev at http://cr.openjdk.java.net/~coleenp/8174749.jdk.01/webrev
> bug link https://bugs.openjdk.java.net/browse/JDK-8174749
>
> Tested with RBT nightly, compiler/jsr292 tests (included in rbt
> nightly), JPRT, jdk/test/java/lang/invoke, jdk/test/java/lang/instrument
> tests.
>
> There are platform dependent changes in this change. They are very
> straightforward, ie. add an indirection to MemberName invocations, but
> could people with access to these platforms test this out for me?
>
> Performance testing showed no regression, and large 1000% improvement
> for the cases that caused us to backout previous attempts at this change.
>
> Thanks,
> Coleen
>