RFR (L) 8174749: Use hash table/oops for MemberName table
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::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.
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 (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)++;
> }
>
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 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.
Okay, yes, now I see it. I added the resolved_method cleaning task to
after string_symbol_task.
>
> ------------------------------------------------------------------------------
>
>
> 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?
>
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 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.
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