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

Stefan Karlsson stefan.karlsson at oracle.com
Fri May 19 12:37:29 UTC 2017

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

More information about the hotspot-dev mailing list