RFR (L) 8174749: Use hash table/oops for MemberName table
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Fri May 19 18:10:43 UTC 2017
On 5/19/17 11:56 AM, Stefan Karlsson wrote:
>
>
> 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.
Thanks, I think we might need to add to this in the future so I'll keep
it simple for now.
>
>>> ------------------------------------------------------------------------------
>>>
>>>
>>>
>>> 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.
thank you!!
Coleen
>
> 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