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

coleen.phillimore at oracle.com coleen.phillimore at oracle.com
Fri May 19 15:05:27 UTC 2017

Stefan, Thank you for reviewing the GC code (and your help).

On 5/19/17 8:37 AM, Stefan Karlsson wrote:

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::unlinkoroopsdo(BoolObjectClosure* isalive, OopClosure* f) { ... 151 if (f != NULL) { 152 f->dooop((oop*)entry->literaladdr()); 153 p = entry->nextaddr(); 154 } else { 155 if (!isalive->doobjectb(entry->literal())) { 156 oopsremoved++; 157 if (logisenabled(Debug, membername, table)) { 158 ResourceMark rm; 159 Method* m = (Method*)javalanginvokeResolvedMethodName::vmtarget(entry->literal()); 160 logdebug(membername, table) ("ResolvedMethod vmtarget entry removed for %s index %d", 161 m->nameandsigasCstring(), i); 162 } 163 *p = entry->next(); 164 thetable->freeentry(entry); 165 } else { 166 p = entry->nextaddr(); 167 } 168 } This code looks backwards to me. If you pass in both an isalive closure and an f (OopClosure), then you ignore the isalive 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.

The reason I did this is because i didn't want to copy the same loop for oops_do() and unlink(), so it's not the same as StringTable. is_alive closure is null for the oops_do case. I'll decouple and just have unlink and oops_do, and if we decide to clear these during copy GC, it can be easily changed to unlink_or_oops_do().

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 (isalive->doobjectb(entry->literal())) { if (f != NULL) { f->dooop((oop*)entry->literaladdr()); } p = entry->nextaddr(); } else { *p = entry->next(); thetable()->freeentry(entry); (*removed)++; }

I can't. is_alive is null when called for oops_do.

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

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 workerid) { 3890 // Do first pass of code cache cleaning. 3891 codecachetask.workfirstpass(workerid); 3892 3893 // Let the threads mark that the first pass is done. 3894 codecachetask.barriermark(workerid); 3895 3896 // Clean the Strings and Symbols. 3897 stringsymboltask.work(workerid); 3898 3899 // Wait for all workers to finish the first code cache cleaning pass. 3900 codecachetask.barrierwait(workerid); 3901 3902 // Do the second code cache cleaning work, which realize on 3903 // the liveness information gathered during the first pass. 3904 codecachetask.worksecondpass(workerid); 3905 3906 // Clean all klasses that were not unloaded. 3907 klasscleaningtask.work(); 3908 3909 // Clean unreferenced things in the ResolvedMethodTable 3910 resolvedmethodcleaningtask.work(); 3911 } The GC workers wait in the barrierwait function as long as there are workers left that have not passed the barriermark point. If you move the resolvedmethodcleaningtask.work() to somewhere between barriermark and barrierwait, there might be some opportunity for one of the workers to do work instead of waiting in the markwait barrier.

Okay, yes, now I see it. I added the resolved_method cleaning task to after string_symbol_task.

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

3876 G1MemberNameCleaningTask resolvedmethodcleaningtask; 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?

I missed that in the renaming. Thank you for catching it.

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

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

I need gcLocker.hpp because NoSafepointVerifier is declared there, but removed the others unnecessary #include.

Webrev with changes tested with my test case (more testing in progress):

open webrev at http://cr.openjdk.java.net/~coleenp/8174749.02/webrev

Thank you!! Coleen

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