Quantcast

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

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore

Hi Serguei,   Thank you for reviewing this.  I should have called you
out to do it :)

On 5/18/17 4:10 AM, [hidden email] wrote:
> Hi Coleen,
>
>
> Nice refactoring!

Thank you!

>
> Some quick comments.
>
>
> http://oklahoma.us.oracle.com/~cphillim/webrev/8174749.01/webrev/src/share/vm/classfile/javaClasses.hpp.udiff.html
>
>   The function name ResolvedMethodName (and the
> j.l.i_ResolvedMethodName) sounds confusing.
>   Would it better name it ResolvedMethod (by its meaning it is
> MemberNameResolvedMethod)?

John and I liked that name.  When I called it ResolvedMethod, in the vm
code it didn't look like an oop.   Also, we're envisioning it to replace
MemberName in some places like LambdaForms and StackWalk frames, so I
wanted to keep the Name in the name.

>
>
> http://oklahoma.us.oracle.com/~cphillim/webrev/8174749.01/webrev/src/share/vm/prims/resolvedMethodTable.hpp.html
>    85   // Called from MethodHandles
>    86   static oop find_method(Method* vmtarget);
>    87   static oop add_method(Handle mem_name_target);
>
>   Do you mean, called from the MethodHandles.cpp ?
>   I do not see these functions are ever called from the MethodHandles.cpp.
>   But they are called from the javaClasses.cpp.

Thanks for noticing that.  I'd moved the code.   Updated the comment like:

   // Called from java_lang_invoke_ResolvedMethodName
   static oop find_method(Method* vmtarget);
   static oop add_method(Handle mem_name_target);

>
>
> http://oklahoma.us.oracle.com/~cphillim/webrev/8174749.01/webrev/src/share/vm/prims/methodHandles.cpp.udiff.html
>   void MethodHandles::expand_MemberName(Handle mname, int suppress, TRAPS) {
>     assert(java_lang_invoke_MemberName::is_instance(mname()), "");
> - Metadata* vmtarget = java_lang_invoke_MemberName::vmtarget(mname());
>     int vmindex  = java_lang_invoke_MemberName::vmindex(mname());
> - if (vmtarget == NULL) {
> + bool have_defc = (java_lang_invoke_MemberName::clazz(mname()) != NULL);
> +
> + assert(have_defc, "defining class should be present");
> +
> + if (!have_defc) {
>       THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), "nothing to expand");
>     }
>  
> - bool have_defc = (java_lang_invoke_MemberName::clazz(mname()) != NULL);
>     bool have_name = (java_lang_invoke_MemberName::name(mname()) != NULL);
>     bool have_type = (java_lang_invoke_MemberName::type(mname()) != NULL);
>     int flags      = java_lang_invoke_MemberName::flags(mname());
>  
>     if (suppress != 0) {
> - if (suppress & _suppress_defc) have_defc = true;
>       if (suppress & _suppress_name)  have_name = true;
>       if (suppress & _suppress_type)  have_type = true;
>     }
>
>    It seems, the assert and THROW_MSG duplicate each other.
>    Also, the _suppress_defc is not used anymore.
>    Should it be removed from the enum in the methodHandles.hpp,
>    or there is a plan to use it later?

There is no plan, I missed that flag.   I had the assert to verify that
the clazz is always initialized properly when the MemberName needs
expansion.
>
>
> http://oklahoma.us.oracle.com/~cphillim/webrev/8174749.01/webrev/src/share/vm/prims/jvmtiRedefineClasses.cpp.udiff.html
>
> + _any_class_has_resolved_methods |= the_class->has_resolved_methods();
>   Should we use '||=' instead of '|=' ?


Yes, fixed.

Thanks,
Coleen

>
>
> Thanks,
> Serguei
>
>
>
>
>
> On 5/17/17 09:01, [hidden email] 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
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Stefan Karlsson
In reply to this post by coleen.phillimore
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, [hidden email] 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
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore

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, [hidden email] 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
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Stefan Karlsson


On 2017-05-19 17:05, [hidden email] 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, [hidden email] 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
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore


On 5/19/17 11:56 AM, Stefan Karlsson wrote:

>
>
> On 2017-05-19 17:05, [hidden email] 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, [hidden email] 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
>>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore
In reply to this post by coleen.phillimore


On 5/22/17 11:23 PM, [hidden email] wrote:
> Hi Coleen,
>
>
> I've finished reviewing, it looks great!

Thank you, Serguei!

>
> Just some nits.
>
> http://cr.openjdk.java.net/~coleenp/8174749.01/webrev/src/share/vm/classfile/javaClasses.cpp.frames.html
>
> A dot is missed at the end of the comment line 3259:
> 3258 // Add a reference to the loader (actually mirror because
> anonymous classes will not have
> 3259 // distinct loaders) to ensure the metadata is kept alive
> 3260 // This mirror may be different than the one in clazz field.
>
ok.

> Dots are also missed in a couple of places in the
> resolvedMethodTable.?pp comments:
>   resolvedMethodTable.hpp: L31, L35
>   resolvedMethodTable.cpp:
>       L130, L141 (L140 has an unnecessary dot)
>

Those comments didn't get fixed when I changed the code.  I wish the
compiler would check this! I fixed them:

// Serially invoke removed unused oops from the table.
// This is done late during GC.
void ResolvedMethodTable::unlink(BoolObjectClosure* is_alive) {

...

// Serially invoke "f->do_oop" on the locations of all oops in the table.
void ResolvedMethodTable::oops_do(OopClosure* f) {
...

> resolvedMethodTable.cpp:
>
>   Values of the counters _oops_removed and_oops_counted are not used.
>   Is it a leftover from the debugging or there was a plan to log them?

They are used for counting and I added this logging message to the end
of ResolvedMethodTable::unlink() to print them.

   log_debug(membername, table) ("ResolvedMethod entries counted %d
removed %d",
                                 _oos_counted, _oops_removed);

>
>   195 // For each entry in MNT, change to new method
>
>   MNT should be RMT now. :)
>

Changed it.  Nice catch.

Thank you for reviewing this!
Coleen

>
> Thanks,
> Serguei
>
>
>
> On 5/17/17 09:01, [hidden email] 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
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

coleen.phillimore


On 5/23/17 10:10 AM, [hidden email] wrote:

>
>
> On 5/22/17 11:23 PM, [hidden email] wrote:
>> Hi Coleen,
>>
>>
>> I've finished reviewing, it looks great!
>
> Thank you, Serguei!
>>
>> Just some nits.
>>
>> http://cr.openjdk.java.net/~coleenp/8174749.01/webrev/src/share/vm/classfile/javaClasses.cpp.frames.html 
>>
>>
>> A dot is missed at the end of the comment line 3259:
>> 3258 // Add a reference to the loader (actually mirror because
>> anonymous classes will not have
>> 3259 // distinct loaders) to ensure the metadata is kept alive
>> 3260 // This mirror may be different than the one in clazz field.
>>
> ok.
>
>> Dots are also missed in a couple of places in the
>> resolvedMethodTable.?pp comments:
>>   resolvedMethodTable.hpp: L31, L35
>>   resolvedMethodTable.cpp:
>>       L130, L141 (L140 has an unnecessary dot)
>>
>
> Those comments didn't get fixed when I changed the code.  I wish the
> compiler would check this! I fixed them:
>
> // Serially invoke removed unused oops from the table.
> // This is done late during GC.
> void ResolvedMethodTable::unlink(BoolObjectClosure* is_alive) {
>
> ...
>
> // Serially invoke "f->do_oop" on the locations of all oops in the table.
> void ResolvedMethodTable::oops_do(OopClosure* f) {
> ...
>
>> resolvedMethodTable.cpp:
>>
>>   Values of the counters _oops_removed and_oops_counted are not used.
>>   Is it a leftover from the debugging or there was a plan to log them?
>
> They are used for counting and I added this logging message to the end
> of ResolvedMethodTable::unlink() to print them.
>
>   log_debug(membername, table) ("ResolvedMethod entries counted %d
> removed %d",
>                                 _oos_counted, _oops_removed);

Make that _oops_counted, _oops_removed.  This is useful logging.
thanks,
Coleen

>
>>
>>   195 // For each entry in MNT, change to new method
>>
>>   MNT should be RMT now. :)
>>
>
> Changed it.  Nice catch.
>
> Thank you for reviewing this!
> Coleen
>
>>
>> Thanks,
>> Serguei
>>
>>
>>
>> On 5/17/17 09:01, [hidden email] 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
>>>
>>
>

Loading...