RFR (L) 8174749: Use hash table/oops for MemberName table
Stefan Karlsson
stefan.karlsson at oracle.com
Fri May 19 15:56:23 UTC 2017
On 2017-05-19 17:05, coleen.phillimore at oracle.com wrote:
>
> 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.
See my comment below:
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.
OK. There are ways to do it anyway.
You could change the code to:
if (is_alive == NULL || is_alive->do_object_b(entry->literal())) {
Or, use the AlwaysTrueClosure, as we do in in jniHandles.hpp:
void JNIHandles::weak_oops_do(OopClosure* f) {
AlwaysTrueClosure always_true;
weak_oops_do(&always_true, f);
}
The proposed decoupling is good as well.
>> ------------------------------------------------------------------------------
>>
>>
>> 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.
Right. Forgot about that one.
>
> Webrev with changes tested with my test case (more testing in progress):
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8174749.02/webrev
The GC parts look good.
Thanks,
StefanK
>
> 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