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

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

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
|

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
|

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
|

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
|

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
|

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
|

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
|

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

Reply | Threaded
Open this post in threaded view
|

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

John Rose-3
In reply to this post by coleen.phillimore
On May 17, 2017, at 9:01 AM, [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

I'm looking at the 8174749.03/webrev version of your changes.

A few comments:

In the JVMCI changes, this line appears to be incorrect on 32-bit machines:

+                vmtargetField = (HotSpotResolvedJavaField) findFieldInClass(methodType, "vmtarget", resolveType(long.class));

(It's a pre-existing condition, and I'm not sure if it is a problem.)

In the new hash table file, the parameter names
seem like they could be made more consistent.

  85 oop ResolvedMethodTable::basic_add(Method* method, oop vmtarget) {

 114 oop ResolvedMethodTable::add_method(Handle mem_name_target) {

I think vmtarget and mem_name_target are the same kind of thing.
Consider renaming them to "entry_to_add" or something more aligned
with the rest of the code.

I don't think that MethodHandles::init_field_MemberName needs TRAPS.
Also, MethodHandles::init_method_MemberName could omit TRAPS if
it were passed the RMN pointer first.  Suggestion:  Remove TRAPS from
both *and* add a trapping function which does the info->RMN step.

  static oop init_method_MemberName(Handle mname_h, CallInfo& info, oop resolved_method);
  static oop init_method_MemberName(Handle mname_h, CallInfo& info, TRAPS);

Then the trapping overloading can pick up the RMN immediately from the info,
and call the non-trapping overloading.  The reason to do something indirect
like this is that the existing code for init_method_MemberName is (a) complex
and (b) non-trapping.  Promoting it all to trapping makes it harder to work with.

In other words, non-TRAPS code is (IMO) easier to read and reason about,
so converting a big method to TRAPS for one line is something I'd like to
avoid.  At least, that's the way I thought about this particular code when
I first wrote it.

Better:  Since init_m_MN is joined at the hip with CallInfo, consider adding the
trapping operation to CallInfo.  See patch below.  I think that preserves CI's
claim to be the Source of Truth for call sites, even in methodHandles.cpp.

Thank you very much for this fix.  I know it's been years since we started
talking about it.  I'm glad you let it bother you enough to fix it!

I looked at everything else and didn't find anything out of place.

Reviewed.

— John

diff --git a/src/share/vm/interpreter/linkResolver.hpp b/src/share/vm/interpreter/linkResolver.hpp
--- a/src/share/vm/interpreter/linkResolver.hpp
+++ b/src/share/vm/interpreter/linkResolver.hpp
@@ -56,6 +56,7 @@
   int          _call_index;             // vtable or itable index of selected class method (if any)
   Handle       _resolved_appendix;      // extra argument in constant pool (if CPCE::has_appendix)
   Handle       _resolved_method_type;   // MethodType (for invokedynamic and invokehandle call sites)
+  Handle       _resolved_method_name;   // optional ResolvedMethodName object for java.lang.invoke
 
   void set_static(KlassHandle resolved_klass, const methodHandle& resolved_method, TRAPS);
   void set_interface(KlassHandle resolved_klass, KlassHandle selected_klass,
@@ -97,6 +98,7 @@
   methodHandle selected_method() const           { return _selected_method; }
   Handle       resolved_appendix() const         { return _resolved_appendix; }
   Handle       resolved_method_type() const      { return _resolved_method_type; }
+  Handle       resolved_method_name() const      { return _resolved_method_name; }
 
   BasicType    result_type() const               { return selected_method()->result_type(); }
   CallKind     call_kind() const                 { return _call_kind; }
@@ -117,6 +119,12 @@
     return _call_index;
   }
 
+  oop find_resolved_method_name(TRAPS) {
+    if (_resolved_method_name.is_null())
+      java_lang_invoke_ResolvedMethodName::find_resolved_method(_resolved_method, CHECK_NULL);
+    return _resolved_method_name;
+  }
+
   // debugging
 #ifdef ASSERT
   bool         has_vtable_index() const          { return _call_index >= 0 && _call_kind != CallInfo::itable_call; }
diff --git a/src/share/vm/interpreter/linkResolver.hpp b/src/share/vm/interpreter/linkResolver.hpp
--- a/src/share/vm/interpreter/linkResolver.hpp
+++ b/src/share/vm/interpreter/linkResolver.hpp
@@ -56,6 +56,7 @@
   int          _call_index;             // vtable or itable index of selected class method (if any)
   Handle       _resolved_appendix;      // extra argument in constant pool (if CPCE::has_appendix)
   Handle       _resolved_method_type;   // MethodType (for invokedynamic and invokehandle call sites)
+  Handle       _resolved_method_name;   // optional ResolvedMethodName object for java.lang.invoke
 
   void set_static(KlassHandle resolved_klass, const methodHandle& resolved_method, TRAPS);
   void set_interface(KlassHandle resolved_klass, KlassHandle selected_klass,
@@ -97,6 +98,7 @@
   methodHandle selected_method() const           { return _selected_method; }
   Handle       resolved_appendix() const         { return _resolved_appendix; }
   Handle       resolved_method_type() const      { return _resolved_method_type; }
+  Handle       resolved_method_name() const      { return _resolved_method_name; }
 
   BasicType    result_type() const               { return selected_method()->result_type(); }
   CallKind     call_kind() const                 { return _call_kind; }
@@ -117,6 +119,12 @@
     return _call_index;
   }
 
+  oop find_resolved_method_name(TRAPS) {
+    if (_resolved_method_name.is_null())
+      java_lang_invoke_ResolvedMethodName::find_resolved_method(_resolved_method, CHECK_NULL);
+    return _resolved_method_name;
+  }
+
   // debugging
 #ifdef ASSERT
   bool         has_vtable_index() const          { return _call_index >= 0 && _call_kind != CallInfo::itable_call; }



Reply | Threaded
Open this post in threaded view
|

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

Doug Simon @ Oracle

> On 26 May 2017, at 02:04, John Rose <[hidden email]> wrote:
>
> On May 17, 2017, at 9:01 AM, [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
>
> I'm looking at the 8174749.03/webrev version of your changes.
>
> A few comments:
>
> In the JVMCI changes, this line appears to be incorrect on 32-bit machines:
>
> +                vmtargetField = (HotSpotResolvedJavaField) findFieldInClass(methodType, "vmtarget", resolveType(long.class));
>
> (It's a pre-existing condition, and I'm not sure if it is a problem.)

Given that JVMCI does not support any 32-bit platforms currently, it should not be a problem in practice. The field lookup would also fail-fast when adding 32-bit support.

That said, we can take this opportunity to make it portable by replacing:

+                vmtargetField = (HotSpotResolvedJavaField) findFieldInClass(methodType, "vmtarget", resolveType(long.class));

with:

+                vmtargetField = (HotSpotResolvedJavaField) findFieldInClass(methodType, "vmtarget", resolveType(HotSpotJVMCIRuntime.getHostWordKind().toJavaClass())));

-Doug

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
In reply to this post by John Rose-3

Hi John,
Thank you for these comments and for your help with this bug fix/RFE.

On 5/25/17 8:04 PM, John Rose wrote:

> On May 17, 2017, at 9:01 AM, [hidden email]
> <mailto:[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
>
> I'm looking at the 8174749.03/webrev version of your changes.
>
> A few comments:
>
> In the JVMCI changes, this line appears to be incorrect on 32-bit
> machines:
>
> +                vmtargetField = (HotSpotResolvedJavaField)
> findFieldInClass(methodType, "vmtarget", resolveType(long.class));
>
> (It's a pre-existing condition, and I'm not sure if it is a problem.)

I'll add a comment. I don't know if Graal supports 32 bits (I'd guess no).

>
> In the new hash table file, the parameter names
> seem like they could be made more consistent.
>
>   85 oop ResolvedMethodTable::basic_add(Method* method, oop vmtarget) {
>
>  114 oop ResolvedMethodTable::add_method(Handle mem_name_target) {
>
> I think vmtarget and mem_name_target are the same kind of thing.
> Consider renaming them to "entry_to_add" or something more aligned
> with the rest of the code.

This code didn't get updated properly from all the renaming.  I've
changed to using "method" when the parameter is Method* and using
rmethod_name when the parameter is an oop representing ResolvedMethodName.

>
> I don't think that MethodHandles::init_field_MemberName needs TRAPS.

Yes, removed TRAPS and reverted the use of the default parameter.

> Also, MethodHandles::init_method_MemberName could omit TRAPS if
> it were passed the RMN pointer first.  Suggestion:  Remove TRAPS from
> both *and* add a trapping function which does the info->RMN step.
>
>   static oop init_method_MemberName(Handle mname_h, CallInfo& info,
> oop resolved_method);
>   static oop init_method_MemberName(Handle mname_h, CallInfo& info,
> TRAPS);
>
> Then the trapping overloading can pick up the RMN immediately from the
> info,
> and call the non-trapping overloading.  The reason to do something
> indirect
> like this is that the existing code for init_method_MemberName is (a)
> complex
> and (b) non-trapping.  Promoting it all to trapping makes it harder to
> work with.
>
> In other words, non-TRAPS code is (IMO) easier to read and reason about,
> so converting a big method to TRAPS for one line is something I'd like to
> avoid.  At least, that's the way I thought about this particular code when
> I first wrote it.
>
> Better:  Since init_m_MN is joined at the hip with CallInfo, consider
> adding the
> trapping operation to CallInfo.  See patch below.  I think that
> preserves CI's
> claim to be the Source of Truth for call sites, even in methodHandles.cpp.
>

This is quite a nice change!   I'll do this and rerun the tests over the
weekend and send out a new version next week.

> Thank you very much for this fix.  I know it's been years since we started
> talking about it.  I'm glad you let it bother you enough to fix it!

We kept running into this, so it was time.

>
> I looked at everything else and didn't find anything out of place.

Thank you!
Coleen

>
> Reviewed.
>
> — John
>
> diff --git a/src/share/vm/interpreter/linkResolver.hpp
> b/src/share/vm/interpreter/linkResolver.hpp
> --- a/src/share/vm/interpreter/linkResolver.hpp
> +++ b/src/share/vm/interpreter/linkResolver.hpp
> @@ -56,6 +56,7 @@
>    int          _call_index;             // vtable or itable index of
> selected class method (if any)
>    Handle       _resolved_appendix;      // extra argument in constant
> pool (if CPCE::has_appendix)
>    Handle       _resolved_method_type;   // MethodType (for
> invokedynamic and invokehandle call sites)
> +  Handle       _resolved_method_name;   // optional
> ResolvedMethodName object for java.lang.invoke
>    void set_static(KlassHandle resolved_klass, const methodHandle&
> resolved_method, TRAPS);
>    void set_interface(KlassHandle resolved_klass, KlassHandle
> selected_klass,
> @@ -97,6 +98,7 @@
>    methodHandle selected_method() const       { return _selected_method; }
>    Handle       resolved_appendix() const       { return
> _resolved_appendix; }
>    Handle       resolved_method_type() const      { return
> _resolved_method_type; }
> +  Handle       resolved_method_name() const      { return
> _resolved_method_name; }
>    BasicType    result_type() const       { return
> selected_method()->result_type(); }
>    CallKind     call_kind() const       { return _call_kind; }
> @@ -117,6 +119,12 @@
>      return _call_index;
>    }
> +  oop find_resolved_method_name(TRAPS) {
> +    if (_resolved_method_name.is_null())
> +
>  java_lang_invoke_ResolvedMethodName::find_resolved_method(_resolved_method,
> CHECK_NULL);
> +    return _resolved_method_name;
> +  }
> +
>    // debugging
>  #ifdef ASSERT
>    bool         has_vtable_index() const      { return _call_index >=
> 0 && _call_kind != CallInfo::itable_call; }
> diff --git a/src/share/vm/interpreter/linkResolver.hpp
> b/src/share/vm/interpreter/linkResolver.hpp
> --- a/src/share/vm/interpreter/linkResolver.hpp
> +++ b/src/share/vm/interpreter/linkResolver.hpp
> @@ -56,6 +56,7 @@
>    int          _call_index;             // vtable or itable index of
> selected class method (if any)
>    Handle       _resolved_appendix;      // extra argument in constant
> pool (if CPCE::has_appendix)
>    Handle       _resolved_method_type;   // MethodType (for
> invokedynamic and invokehandle call sites)
> +  Handle       _resolved_method_name;   // optional
> ResolvedMethodName object for java.lang.invoke
>    void set_static(KlassHandle resolved_klass, const methodHandle&
> resolved_method, TRAPS);
>    void set_interface(KlassHandle resolved_klass, KlassHandle
> selected_klass,
> @@ -97,6 +98,7 @@
>    methodHandle selected_method() const       { return _selected_method; }
>    Handle       resolved_appendix() const       { return
> _resolved_appendix; }
>    Handle       resolved_method_type() const      { return
> _resolved_method_type; }
> +  Handle       resolved_method_name() const      { return
> _resolved_method_name; }
>    BasicType    result_type() const       { return
> selected_method()->result_type(); }
>    CallKind     call_kind() const       { return _call_kind; }
> @@ -117,6 +119,12 @@
>      return _call_index;
>    }
> +  oop find_resolved_method_name(TRAPS) {
> +    if (_resolved_method_name.is_null())
> +
>  java_lang_invoke_ResolvedMethodName::find_resolved_method(_resolved_method,
> CHECK_NULL);
> +    return _resolved_method_name;
> +  }
> +
>    // debugging
>  #ifdef ASSERT
>    bool         has_vtable_index() const      { return _call_index >=
> 0 && _call_kind != CallInfo::itable_call; }
>
>
>

Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
In reply to this post by Doug Simon @ Oracle


On 5/26/17 5:48 AM, Doug Simon wrote:

>> On 26 May 2017, at 02:04, John Rose <[hidden email]> wrote:
>>
>> On May 17, 2017, at 9:01 AM, [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
>> I'm looking at the 8174749.03/webrev version of your changes.
>>
>> A few comments:
>>
>> In the JVMCI changes, this line appears to be incorrect on 32-bit machines:
>>
>> +                vmtargetField = (HotSpotResolvedJavaField) findFieldInClass(methodType, "vmtarget", resolveType(long.class));
>>
>> (It's a pre-existing condition, and I'm not sure if it is a problem.)
> Given that JVMCI does not support any 32-bit platforms currently, it should not be a problem in practice. The field lookup would also fail-fast when adding 32-bit support.
>
> That said, we can take this opportunity to make it portable by replacing:
>
> +                vmtargetField = (HotSpotResolvedJavaField) findFieldInClass(methodType, "vmtarget", resolveType(long.class));
>
> with:
>
> +                vmtargetField = (HotSpotResolvedJavaField) findFieldInClass(methodType, "vmtarget", resolveType(HotSpotJVMCIRuntime.getHostWordKind().toJavaClass())));

I can make this change.  I hope it works after I cut and paste this :)
Coleen

>
> -Doug
>

Reply | Threaded
Open this post in threaded view
|

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

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

Hi, I made the changes below, which turned out very nice.  It didn't
take that long to retest.  See:

open webrev at http://cr.openjdk.java.net/~coleenp/8174749.04/webrev
open webrev at http://cr.openjdk.java.net/~coleenp/8174749.jdk.04/webrev

I don't know how to do delta webrevs, so just look at
linkResolver.cpp/hpp and methodHandles.cpp

Thanks,
Coleen

On 5/26/17 9:37 AM, [hidden email] wrote:

>
> Hi John,
> Thank you for these comments and for your help with this bug fix/RFE.
>
> On 5/25/17 8:04 PM, John Rose wrote:
>> On May 17, 2017, at 9:01 AM, [hidden email]
>> <mailto:[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
>>
>> I'm looking at the 8174749.03/webrev version of your changes.
>>
>> A few comments:
>>
>> In the JVMCI changes, this line appears to be incorrect on 32-bit
>> machines:
>>
>> +                vmtargetField = (HotSpotResolvedJavaField)
>> findFieldInClass(methodType, "vmtarget", resolveType(long.class));
>>
>> (It's a pre-existing condition, and I'm not sure if it is a problem.)
>
> I'll add a comment. I don't know if Graal supports 32 bits (I'd guess
> no).
>>
>> In the new hash table file, the parameter names
>> seem like they could be made more consistent.
>>
>>   85 oop ResolvedMethodTable::basic_add(Method* method, oop vmtarget) {
>>
>>  114 oop ResolvedMethodTable::add_method(Handle mem_name_target) {
>>
>> I think vmtarget and mem_name_target are the same kind of thing.
>> Consider renaming them to "entry_to_add" or something more aligned
>> with the rest of the code.
>
> This code didn't get updated properly from all the renaming.  I've
> changed to using "method" when the parameter is Method* and using
> rmethod_name when the parameter is an oop representing
> ResolvedMethodName.
>
>>
>> I don't think that MethodHandles::init_field_MemberName needs TRAPS.
>
> Yes, removed TRAPS and reverted the use of the default parameter.
>> Also, MethodHandles::init_method_MemberName could omit TRAPS if
>> it were passed the RMN pointer first.  Suggestion:  Remove TRAPS from
>> both *and* add a trapping function which does the info->RMN step.
>>
>>   static oop init_method_MemberName(Handle mname_h, CallInfo& info,
>> oop resolved_method);
>>   static oop init_method_MemberName(Handle mname_h, CallInfo& info,
>> TRAPS);
>>
>> Then the trapping overloading can pick up the RMN immediately from
>> the info,
>> and call the non-trapping overloading.  The reason to do something
>> indirect
>> like this is that the existing code for init_method_MemberName is (a)
>> complex
>> and (b) non-trapping.  Promoting it all to trapping makes it harder
>> to work with.
>>
>> In other words, non-TRAPS code is (IMO) easier to read and reason about,
>> so converting a big method to TRAPS for one line is something I'd
>> like to
>> avoid.  At least, that's the way I thought about this particular code
>> when
>> I first wrote it.
>>
>> Better:  Since init_m_MN is joined at the hip with CallInfo, consider
>> adding the
>> trapping operation to CallInfo.  See patch below.  I think that
>> preserves CI's
>> claim to be the Source of Truth for call sites, even in
>> methodHandles.cpp.
>>
>
> This is quite a nice change!   I'll do this and rerun the tests over
> the weekend and send out a new version next week.
>
>> Thank you very much for this fix.  I know it's been years since we
>> started
>> talking about it.  I'm glad you let it bother you enough to fix it!
>
> We kept running into this, so it was time.
>
>>
>> I looked at everything else and didn't find anything out of place.
>
> Thank you!
> Coleen
>
>>
>> Reviewed.
>>
>> — John
>>
>> diff --git a/src/share/vm/interpreter/linkResolver.hpp
>> b/src/share/vm/interpreter/linkResolver.hpp
>> --- a/src/share/vm/interpreter/linkResolver.hpp
>> +++ b/src/share/vm/interpreter/linkResolver.hpp
>> @@ -56,6 +56,7 @@
>>    int          _call_index;             // vtable or itable index of
>> selected class method (if any)
>>    Handle       _resolved_appendix;      // extra argument in
>> constant pool (if CPCE::has_appendix)
>>    Handle       _resolved_method_type;   // MethodType (for
>> invokedynamic and invokehandle call sites)
>> +  Handle       _resolved_method_name;   // optional
>> ResolvedMethodName object for java.lang.invoke
>>    void set_static(KlassHandle resolved_klass, const methodHandle&
>> resolved_method, TRAPS);
>>    void set_interface(KlassHandle resolved_klass, KlassHandle
>> selected_klass,
>> @@ -97,6 +98,7 @@
>>    methodHandle selected_method() const       { return
>> _selected_method; }
>>    Handle       resolved_appendix() const       { return
>> _resolved_appendix; }
>>    Handle       resolved_method_type() const      { return
>> _resolved_method_type; }
>> +  Handle       resolved_method_name() const      { return
>> _resolved_method_name; }
>>    BasicType    result_type() const       { return
>> selected_method()->result_type(); }
>>    CallKind     call_kind() const       { return _call_kind; }
>> @@ -117,6 +119,12 @@
>>      return _call_index;
>>    }
>> +  oop find_resolved_method_name(TRAPS) {
>> +    if (_resolved_method_name.is_null())
>> +
>>  java_lang_invoke_ResolvedMethodName::find_resolved_method(_resolved_method,
>> CHECK_NULL);
>> +    return _resolved_method_name;
>> +  }
>> +
>>    // debugging
>>  #ifdef ASSERT
>>    bool         has_vtable_index() const      { return _call_index >=
>> 0 && _call_kind != CallInfo::itable_call; }
>> diff --git a/src/share/vm/interpreter/linkResolver.hpp
>> b/src/share/vm/interpreter/linkResolver.hpp
>> --- a/src/share/vm/interpreter/linkResolver.hpp
>> +++ b/src/share/vm/interpreter/linkResolver.hpp
>> @@ -56,6 +56,7 @@
>>    int          _call_index;             // vtable or itable index of
>> selected class method (if any)
>>    Handle       _resolved_appendix;      // extra argument in
>> constant pool (if CPCE::has_appendix)
>>    Handle       _resolved_method_type;   // MethodType (for
>> invokedynamic and invokehandle call sites)
>> +  Handle       _resolved_method_name;   // optional
>> ResolvedMethodName object for java.lang.invoke
>>    void set_static(KlassHandle resolved_klass, const methodHandle&
>> resolved_method, TRAPS);
>>    void set_interface(KlassHandle resolved_klass, KlassHandle
>> selected_klass,
>> @@ -97,6 +98,7 @@
>>    methodHandle selected_method() const       { return
>> _selected_method; }
>>    Handle       resolved_appendix() const       { return
>> _resolved_appendix; }
>>    Handle       resolved_method_type() const      { return
>> _resolved_method_type; }
>> +  Handle       resolved_method_name() const      { return
>> _resolved_method_name; }
>>    BasicType    result_type() const       { return
>> selected_method()->result_type(); }
>>    CallKind     call_kind() const       { return _call_kind; }
>> @@ -117,6 +119,12 @@
>>      return _call_index;
>>    }
>> +  oop find_resolved_method_name(TRAPS) {
>> +    if (_resolved_method_name.is_null())
>> +
>>  java_lang_invoke_ResolvedMethodName::find_resolved_method(_resolved_method,
>> CHECK_NULL);
>> +    return _resolved_method_name;
>> +  }
>> +
>>    // debugging
>>  #ifdef ASSERT
>>    bool         has_vtable_index() const      { return _call_index >=
>> 0 && _call_kind != CallInfo::itable_call; }
>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

John Rose-3
One more comment.  I am not sure about this change:

   int vmindex  = java_lang_invoke_MemberName::vmindex(mname());
-  if (vmtarget == NULL) {
+  bool have_defc = (java_lang_invoke_MemberName::clazz(mname()) != NULL);
+
+  if (!have_defc) {
     THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), "nothing to expand");
   }


The point of the expander is that if a MN has *only* a RMN, *then* it can recover the rest of its guts.

That's why the old code checked vmtarget, and new code should, I think, check the new 'method' for non-null.

The point is that if you are holding an RMN, you can't "peek" into it unless you wrap a blank MN around it and request "expand".

I hesitated to mention this earlier because I assume there was another reason for changing the logic of this method, but now after a full review I don't see the reason.

Notice that RMN is now a completely opaque Java type, which is very good.  But there needs to be a Java API for unpacking an RMN, and MN.expand is that API.  This is where we came out in the end, and I think the have_defc changes are now just an artifact of an intermediate state.

So, I suggest walking back the changes related to "have_defc", unless I've missed something here.

— John
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore


On 5/26/17 4:01 PM, John Rose wrote:

> One more comment.  I am not sure about this change:
>
>     int vmindex  = java_lang_invoke_MemberName::vmindex(mname());
> - if (vmtarget == NULL) {
> + bool have_defc = (java_lang_invoke_MemberName::clazz(mname()) != NULL);
> +
> + if (!have_defc) {
>       THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), "nothing to expand");
>     }
>
>
> The point of the expander is that if a MN has *only* a RMN, *then* it
> can recover the rest of its guts.
>
> That's why the old code checked vmtarget, and new code should, I
> think, check the new 'method' for non-null.

I made this change so that field MemberName doesn't have to have a
vmtarget, since now it's a ResolvedMethodName.  For field MemberName,
the "method" field is null.

I found that the clazz field is properly initialized in init_MemberName
and never changes and is never null here, so that's why I check the clazz.

>
> The point is that if you are holding an RMN, you can't "peek" into it
> unless you wrap a blank MN around it and request "expand".
>
> I hesitated to mention this earlier because I assume there was another
> reason for changing the logic of this method, but now after a full
> review I don't see the reason.
>
> Notice that RMN is now a completely opaque Java type, which is very
> good.  But there needs to be a Java API for unpacking an RMN, and
> MN.expand is that API.  This is where we came out in the end, and I
> think the have_defc changes are now just an artifact of an
> intermediate state.
>
> So, I suggest walking back the changes related to "have_defc", unless
> I've missed something here.

I thought we should add a different API for expanding a MemberName when
you are holding a RMN, that isn't applicable for field MemberName.  
That's why I made this change.

Coleen

>
> — John

Reply | Threaded
Open this post in threaded view
|

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

John Rose-3
In reply to this post by coleen.phillimore
On May 26, 2017, at 10:47 AM, [hidden email] wrote:
>
> Hi, I made the changes below, which turned out very nice.  It didn't take that long to retest.  See:
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8174749.04/webrev <http://cr.openjdk.java.net/~coleenp/8174749.04/webrev>
> open webrev at http://cr.openjdk.java.net/~coleenp/8174749.jdk.04/webrev <http://cr.openjdk.java.net/~coleenp/8174749.jdk.04/webrev>
>
> I don't know how to do delta webrevs, so just look at linkResolver.cpp/hpp and methodHandles.cpp

Re-reviewed.

See previous message for a late-breaking comment on expand.
See below for a sketch of what I mean by keeping "have_defc" as is.

(Another reviewer commented about a dead mode bit.  The purpose of that
stuff is to allow us to tweak the JDK API.  I don't care much either way about
GC-ing unused mode bits but I do want to keep the expander capability so we
can prototype stuff in the JDK without having to edit the JVM.  So on balance,
I'd give the mode bits the benefit of the doubt.  They can be used from the JDK,
even if they aren't at the moment.)

I also like how this CallInfo change turned out.  Notice how now the function
java_lang_invoke_ResolvedMethodName::find_resolved_method has only
one usage, from the inside of CallInfo.  This feels right.  It also means you
can take javaClasses.cpp out of the loop here, and just have CallInfo call
directly into SystemDictionary and ResolvedMethodTable.  It seems just
as reasonable to me that linkResolver.cpp would do that job, than that it
would be to delegate via javaClasses.cpp.  I also think the patch will get
a little smaller if you cut javaClasses.cpp out of that loop.

Thanks,
— John

P.S. As a step after this fix, if we loosen the coupling of the JVM with MemberName,
I think we will want to get rid of MN::vmtarget and just have MN::method.
In the code of MHN_getMemberVMInfo, the unchanged line "x = mname()"
really wants to be "x = method" where method is the RMN.  The JDK code
expects a MN at that point, but it should really be the RMN now.  The only
JDK change would be in MemberName.java:

-            assert(vmtarget instanceof MemberName) : vmtarget + " in " + this;
+            assert(vmtarget instanceof ResolvedMethodName) : vmtarget + " in " + this;

I wouldn't object if you anticipated this in the present change set, but it's OK
to do it later.

P.P.S.  Here's a sketch of what I mean by walking back some of the "have_defc"
changes.  Maybe I'm missing something, but I think this version makes more
sense than the current version:

git a/src/share/vm/prims/methodHandles.cpp b/src/share/vm/prims/methodHandles.cpp
--- a/src/share/vm/prims/methodHandles.cpp
+++ b/src/share/vm/prims/methodHandles.cpp
@@ -794,11 +794,6 @@
 // which refers directly to JVM internals.
 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) {
-    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);
@@ -817,10 +812,14 @@
   case IS_METHOD:
   case IS_CONSTRUCTOR:
     {
+      Method* vmtarget = java_lang_invoke_MemberName::vmtarget(method());
+      if (vmtarget == NULL) {
+        THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), "nothing to expand");
+      }
       assert(vmtarget->is_method(), "method or constructor vmtarget is Method*");
       methodHandle m(THREAD, (Method*)vmtarget);
       DEBUG_ONLY(vmtarget = NULL);  // safety
-      if (m.is_null())  break;
+      assert(m.not_null(), "checked above");
       if (!have_defc) {
         InstanceKlass* defc = m->method_holder();
         java_lang_invoke_MemberName::set_clazz(mname(), defc->java_mirror());
@@ -838,17 +837,16 @@
     }
   case IS_FIELD:
     {
-      assert(vmtarget->is_klass(), "field vmtarget is Klass*");
-      if (!((Klass*) vmtarget)->is_instance_klass())  break;
-      instanceKlassHandle defc(THREAD, (Klass*) vmtarget);
-      DEBUG_ONLY(vmtarget = NULL);  // safety
+      oop clazz = java_lang_invoke_MemberName::clazz(mname());
+      if (clazz == NULL) {
+        THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), "nothing to expand (as field)");
+      }
+      InstanceKlass* defc = InstanceKlass::cast(java_lang_Class::as_Klass(clazz));
+      DEBUG_ONLY(clazz = NULL);  // safety
       bool is_static = ((flags & JVM_ACC_STATIC) != 0);
       fieldDescriptor fd; // find_field initializes fd if found
       if (!defc->find_field_from_offset(vmindex, is_static, &fd))
         break;                  // cannot expand
-      if (!have_defc) {
-        java_lang_invoke_MemberName::set_clazz(mname(), defc->java_mirror());
-      }
       if (!have_name) {
         //not java_lang_String::create_from_symbol; let's intern member names
         Handle name = StringTable::intern(fd.name(), CHECK);
@@ -1389,6 +1387,39 @@

Reply | Threaded
Open this post in threaded view
|

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

John Rose-3
In reply to this post by coleen.phillimore
On May 26, 2017, at 1:32 PM, [hidden email] wrote:
>
> I thought we should add a different API for expanding a MemberName when you are holding a RMN, that isn't applicable for field MemberName.  That's why I made this change.

I see.  Well, that would in fact replace the whole MN.expand thing,
since that's what it's for.  Until we have the different API, let's keep
the old one intact.  Note the differing treatment of fields and methods
in the patch I suggested; I think that makes sense.

— John
Reply | Threaded
Open this post in threaded view
|

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

coleen.phillimore
In reply to this post by John Rose-3


On 5/26/17 4:48 PM, John Rose wrote:

> On May 26, 2017, at 10:47 AM, [hidden email]
> <mailto:[hidden email]> wrote:
>>
>> Hi, I made the changes below, which turned out very nice.  It didn't
>> take that long to retest.  See:
>>
>> open webrev athttp://cr.openjdk.java.net/~coleenp/8174749.04/webrev
>> <http://cr.openjdk.java.net/%7Ecoleenp/8174749.04/webrev>
>> open webrev
>> athttp://cr.openjdk.java.net/~coleenp/8174749.jdk.04/webrev
>> <http://cr.openjdk.java.net/%7Ecoleenp/8174749.jdk.04/webrev>
>>
>> I don't know how to do delta webrevs, so just look at
>> linkResolver.cpp/hpp and methodHandles.cpp
>
> Re-reviewed.
>
> See previous message for a late-breaking comment on expand.
> See below for a sketch of what I mean by keeping "have_defc" as is.
Hi John,

I was just thinking of this change below, it makes sense to treat field
and method MemberName differently as you have below.  The field needs
the clazz present to be expanded but method MemberName does not.

Yes, this makes sense.

>
> (Another reviewer commented about a dead mode bit.  The purpose of that
> stuff is to allow us to tweak the JDK API.  I don't care much either
> way about
> GC-ing unused mode bits but I do want to keep the expander capability
> so we
> can prototype stuff in the JDK without having to edit the JVM.  So on
> balance,
> I'd give the mode bits the benefit of the doubt.  They can be used
> from the JDK,
> even if they aren't at the moment.)
>
> I also like how this CallInfo change turned out.  Notice how now the
> function
> java_lang_invoke_ResolvedMethodName::find_resolved_method has only
> one usage, from the inside of CallInfo.  This feels right.  It also
> means you
> can take javaClasses.cpp out of the loop here, and just have CallInfo call
> directly into SystemDictionary and ResolvedMethodTable.  It seems just
> as reasonable to me that linkResolver.cpp would do that job, than that it
> would be to delegate via javaClasses.cpp.  I also think the patch will get
> a little smaller if you cut javaClasses.cpp out of that loop.

JavaClasses is in the loop because it knows which fields to assign and
how to create a ResolvedMethodName.  I think this makes sense to isolate
it like this an appreciated only changing javaClasses.cpp when I kept
changing the names of the fields.

>
> Thanks,
> — John
>
> P.S. As a step after this fix, if we loosen the coupling of the JVM
> with MemberName,
> I think we will want to get rid of MN::vmtarget and just have MN::method.
> In the code of MHN_getMemberVMInfo, the unchanged line "x = mname()"
> really wants to be "x = method" where method is the RMN.  The JDK code
> expects a MN at that point, but it should really be the RMN now.  The only
> JDK change would be in MemberName.java:
>
> -            assert(vmtarget instanceof MemberName) : vmtarget + " in
> " + this;
> +            assert(vmtarget instanceof ResolvedMethodName) : vmtarget
> + " in " + this;
>
> I wouldn't object if you anticipated this in the present change set,
> but it's OK
> to do it later.

Yes, it has to be later.  I'm going to file a couple of RFE's after this
that we discussed so that RMN can be used instead of MN.  And believe it
or not, large changes make me anxious.  :)
>
> P.P.S.  Here's a sketch of what I mean by walking back some of the
> "have_defc"
> changes.  Maybe I'm missing something, but I think this version makes more
> sense than the current version:

Done.   Passes java/lang/invoke tests (as sanity).

http://cr.openjdk.java.net/~coleenp/8174749.05/webrev

(wish I could do incremental webrevs because full webrevs take forever).

Thank you for all your help and comments.

Coleen


>
> git a/src/share/vm/prims/methodHandles.cpp
> b/src/share/vm/prims/methodHandles.cpp
> --- a/src/share/vm/prims/methodHandles.cpp
> +++ b/src/share/vm/prims/methodHandles.cpp
> @@ -794,11 +794,6 @@
>  // which refers directly to JVM internals.
>  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) {
> -  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);
> @@ -817,10 +812,14 @@
>    case IS_METHOD:
>    case IS_CONSTRUCTOR:
>      {
> +      Method* vmtarget = java_lang_invoke_MemberName::vmtarget(method());
> +      if (vmtarget == NULL) {
> +  THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), "nothing
> to expand");
> +      }
>        assert(vmtarget->is_method(), "method or constructor vmtarget
> is Method*");
>        methodHandle m(THREAD, (Method*)vmtarget);
>        DEBUG_ONLY(vmtarget = NULL);  // safety
> -      if (m.is_null())  break;
> +      assert(m.not_null(), "checked above");
>        if (!have_defc) {
>          InstanceKlass* defc = m->method_holder();
>  java_lang_invoke_MemberName::set_clazz(mname(), defc->java_mirror());
> @@ -838,17 +837,16 @@
>      }
>    case IS_FIELD:
>      {
> -      assert(vmtarget->is_klass(), "field vmtarget is Klass*");
> -      if (!((Klass*) vmtarget)->is_instance_klass())  break;
> -      instanceKlassHandle defc(THREAD, (Klass*) vmtarget);
> -      DEBUG_ONLY(vmtarget = NULL);  // safety
> +      oop clazz = java_lang_invoke_MemberName::clazz(mname());
> +      if (clazz == NULL) {
> +  THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(), "nothing
> to expand (as field)");
> +      }
> +      InstanceKlass* defc =
> InstanceKlass::cast(java_lang_Class::as_Klass(clazz));
> +      DEBUG_ONLY(clazz = NULL);  // safety
>        bool is_static = ((flags & JVM_ACC_STATIC) != 0);
>        fieldDescriptor fd; // find_field initializes fd if found
>        if (!defc->find_field_from_offset(vmindex, is_static, &fd))
>          break;                  // cannot expand
> -      if (!have_defc) {
> -  java_lang_invoke_MemberName::set_clazz(mname(), defc->java_mirror());
> -      }
>        if (!have_name) {
>          //not java_lang_String::create_from_symbol; let's intern
> member names
>          Handle name = StringTable::intern(fd.name(), CHECK);
> @@ -1389,6 +1387,39 @@
>

Reply | Threaded
Open this post in threaded view
|

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

John Rose-3
On May 26, 2017, at 2:48 PM, [hidden email] wrote:

>
> On 5/26/17 4:48 PM, John Rose wrote:
>> On May 26, 2017, at 10:47 AM, [hidden email] <mailto:[hidden email]> wrote:
>>>
>>> Hi, I made the changes below, which turned out very nice.  It didn't take that long to retest.  See:
>>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8174749.04/webrev <http://cr.openjdk.java.net/%7Ecoleenp/8174749.04/webrev>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/8174749.jdk.04/webrev <http://cr.openjdk.java.net/%7Ecoleenp/8174749.jdk.04/webrev>
>>>
>>> I don't know how to do delta webrevs, so just look at linkResolver.cpp/hpp and methodHandles.cpp
>>
>> Re-reviewed.
>>
>> See previous message for a late-breaking comment on expand.
>> See below for a sketch of what I mean by keeping "have_defc" as is.
> Hi John,
>
> I was just thinking of this change below, it makes sense to treat field and method MemberName differently as you have below.  The field needs the clazz present to be expanded but method MemberName does not.
>
> Yes, this makes sense.

Excellent. I reviewed the change in the 05 version of the webrev and it looks good to go.

>>
>> ...It seems just
>> as reasonable to me that linkResolver.cpp would do that job, than that it
>> would be to delegate via javaClasses.cpp.  I also think the patch will get
>> a little smaller if you cut javaClasses.cpp out of that loop.
>
> JavaClasses is in the loop because it knows which fields to assign and how to create a ResolvedMethodName.  I think this makes sense to isolate it like this an appreciated only changing javaClasses.cpp when I kept changing the names of the fields.

OK, then keep it as is.  The allocate_instance call on RMN seems at home in jC.cpp.

>> I wouldn't object if you anticipated this in the present change set, but it's OK
>> to do it later.
>
> Yes, it has to be later.  I'm going to file a couple of RFE's after this that we discussed so that RMN can be used instead of MN.  And believe it or not, large changes make me anxious.  :)

Yep.  An RFE to track this makes me happy.

>>
>> P.P.S.  Here's a sketch of what I mean by walking back some of the "have_defc"
>> changes.  Maybe I'm missing something, but I think this version makes more
>> sense than the current version:
>
> Done.   Passes java/lang/invoke tests (as sanity).
>
> http://cr.openjdk.java.net/~coleenp/8174749.05/webrev <http://cr.openjdk.java.net/~coleenp/8174749.05/webrev>
>
> (wish I could do incremental webrevs because full webrevs take forever).
>
> Thank you for all your help and comments.

Re-reviewed.  Even better, and still good to go.

— John

Reply | Threaded
Open this post in threaded view
|

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

serguei.spitsyn@oracle.com
In reply to this post by coleen.phillimore
Hi Coleen,

It looks good to me.
At least, I do not see anything bad in the last update.

Thanks,
Serguei


On 5/26/17 14:48, [hidden email] wrote:

>
>
> On 5/26/17 4:48 PM, John Rose wrote:
>> On May 26, 2017, at 10:47 AM, [hidden email]
>> <mailto:[hidden email]> wrote:
>>>
>>> Hi, I made the changes below, which turned out very nice.  It didn't
>>> take that long to retest.  See:
>>>
>>> open webrev athttp://cr.openjdk.java.net/~coleenp/8174749.04/webrev
>>> <http://cr.openjdk.java.net/%7Ecoleenp/8174749.04/webrev>
>>> open webrev
>>> athttp://cr.openjdk.java.net/~coleenp/8174749.jdk.04/webrev
>>> <http://cr.openjdk.java.net/%7Ecoleenp/8174749.jdk.04/webrev>
>>>
>>> I don't know how to do delta webrevs, so just look at
>>> linkResolver.cpp/hpp and methodHandles.cpp
>>
>> Re-reviewed.
>>
>> See previous message for a late-breaking comment on expand.
>> See below for a sketch of what I mean by keeping "have_defc" as is.
> Hi John,
>
> I was just thinking of this change below, it makes sense to treat
> field and method MemberName differently as you have below.  The field
> needs the clazz present to be expanded but method MemberName does not.
>
> Yes, this makes sense.
>>
>> (Another reviewer commented about a dead mode bit.  The purpose of that
>> stuff is to allow us to tweak the JDK API.  I don't care much either
>> way about
>> GC-ing unused mode bits but I do want to keep the expander capability
>> so we
>> can prototype stuff in the JDK without having to edit the JVM. So on
>> balance,
>> I'd give the mode bits the benefit of the doubt.  They can be used
>> from the JDK,
>> even if they aren't at the moment.)
>>
>> I also like how this CallInfo change turned out.  Notice how now the
>> function
>> java_lang_invoke_ResolvedMethodName::find_resolved_method has only
>> one usage, from the inside of CallInfo.  This feels right.  It also
>> means you
>> can take javaClasses.cpp out of the loop here, and just have CallInfo
>> call
>> directly into SystemDictionary and ResolvedMethodTable.  It seems just
>> as reasonable to me that linkResolver.cpp would do that job, than
>> that it
>> would be to delegate via javaClasses.cpp.  I also think the patch
>> will get
>> a little smaller if you cut javaClasses.cpp out of that loop.
>
> JavaClasses is in the loop because it knows which fields to assign and
> how to create a ResolvedMethodName.  I think this makes sense to
> isolate it like this an appreciated only changing javaClasses.cpp when
> I kept changing the names of the fields.
>
>>
>> Thanks,
>> — John
>>
>> P.S. As a step after this fix, if we loosen the coupling of the JVM
>> with MemberName,
>> I think we will want to get rid of MN::vmtarget and just have
>> MN::method.
>> In the code of MHN_getMemberVMInfo, the unchanged line "x = mname()"
>> really wants to be "x = method" where method is the RMN.  The JDK code
>> expects a MN at that point, but it should really be the RMN now.  The
>> only
>> JDK change would be in MemberName.java:
>>
>> -            assert(vmtarget instanceof MemberName) : vmtarget + " in
>> " + this;
>> +            assert(vmtarget instanceof ResolvedMethodName) :
>> vmtarget + " in " + this;
>>
>> I wouldn't object if you anticipated this in the present change set,
>> but it's OK
>> to do it later.
>
> Yes, it has to be later.  I'm going to file a couple of RFE's after
> this that we discussed so that RMN can be used instead of MN.  And
> believe it or not, large changes make me anxious.  :)
>>
>> P.P.S.  Here's a sketch of what I mean by walking back some of the
>> "have_defc"
>> changes.  Maybe I'm missing something, but I think this version makes
>> more
>> sense than the current version:
>
> Done.   Passes java/lang/invoke tests (as sanity).
>
> http://cr.openjdk.java.net/~coleenp/8174749.05/webrev
>
> (wish I could do incremental webrevs because full webrevs take forever).
>
> Thank you for all your help and comments.
>
> Coleen
>
>
>>
>> git a/src/share/vm/prims/methodHandles.cpp
>> b/src/share/vm/prims/methodHandles.cpp
>> --- a/src/share/vm/prims/methodHandles.cpp
>> +++ b/src/share/vm/prims/methodHandles.cpp
>> @@ -794,11 +794,6 @@
>>  // which refers directly to JVM internals.
>>  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) {
>> -  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);
>> @@ -817,10 +812,14 @@
>>    case IS_METHOD:
>>    case IS_CONSTRUCTOR:
>>      {
>> +      Method* vmtarget =
>> java_lang_invoke_MemberName::vmtarget(method());
>> +      if (vmtarget == NULL) {
>> +  THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
>> "nothing to expand");
>> +      }
>>        assert(vmtarget->is_method(), "method or constructor vmtarget
>> is Method*");
>>        methodHandle m(THREAD, (Method*)vmtarget);
>>        DEBUG_ONLY(vmtarget = NULL);  // safety
>> -      if (m.is_null())  break;
>> +      assert(m.not_null(), "checked above");
>>        if (!have_defc) {
>>          InstanceKlass* defc = m->method_holder();
>>  java_lang_invoke_MemberName::set_clazz(mname(), defc->java_mirror());
>> @@ -838,17 +837,16 @@
>>      }
>>    case IS_FIELD:
>>      {
>> -      assert(vmtarget->is_klass(), "field vmtarget is Klass*");
>> -      if (!((Klass*) vmtarget)->is_instance_klass())  break;
>> -      instanceKlassHandle defc(THREAD, (Klass*) vmtarget);
>> -      DEBUG_ONLY(vmtarget = NULL);  // safety
>> +      oop clazz = java_lang_invoke_MemberName::clazz(mname());
>> +      if (clazz == NULL) {
>> +  THROW_MSG(vmSymbols::java_lang_IllegalArgumentException(),
>> "nothing to expand (as field)");
>> +      }
>> +      InstanceKlass* defc =
>> InstanceKlass::cast(java_lang_Class::as_Klass(clazz));
>> +      DEBUG_ONLY(clazz = NULL);  // safety
>>        bool is_static = ((flags & JVM_ACC_STATIC) != 0);
>>        fieldDescriptor fd; // find_field initializes fd if found
>>        if (!defc->find_field_from_offset(vmindex, is_static, &fd))
>>          break;                  // cannot expand
>> -      if (!have_defc) {
>> -  java_lang_invoke_MemberName::set_clazz(mname(), defc->java_mirror());
>> -      }
>>        if (!have_name) {
>>          //not java_lang_String::create_from_symbol; let's intern
>> member names
>>          Handle name = StringTable::intern(fd.name(), CHECK);
>> @@ -1389,6 +1387,39 @@
>>
>