<html>
<head>
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<div class="moz-cite-prefix">On 04/02/2013 01:02 PM,
<a class="moz-txt-link-abbreviated" href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a> wrote:<br>
</div>
<blockquote cite="mid:515B0F17.5040803@oracle.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 4/1/13 3:51 PM, Coleen Phillimore
wrote:<br>
</div>
<blockquote cite="mid:515A0F60.1040105@oracle.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<br>
<div class="moz-cite-prefix">On 04/01/2013 06:02 PM, <a
moz-do-not-send="true" class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote cite="mid:515A03FB.30808@oracle.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<div class="moz-cite-prefix">On 4/1/13 1:09 PM, Coleen
Phillimore wrote:<br>
</div>
<blockquote cite="mid:5159E95E.2030705@oracle.com" type="cite">
<meta content="text/html; charset=windows-1252"
http-equiv="Content-Type">
<br>
Hi Serguei,<br>
<br>
Sorry for the delay in reviewing this.<br>
<br>
In instanceKlass.cpp line 2730, can you make that mtClass
since it's part of class metadata?<br>
</blockquote>
<br>
Fixed.<br>
Thank you for the catch!<br>
<blockquote cite="mid:5159E95E.2030705@oracle.com" type="cite">
<br>
In methodHandles.cpp, I think the whole member name table
mechanism could be protected by #if INCLUDE_JVMTI since it's
not needed unless you have jvmti built in the vm. And you
don't need to call add_method_handle() or just have it do
JVMTI_RETURN or something for the minimal vm.<br>
</blockquote>
<br>
My understanding was that John is considering to use the<br>
MemberNameTable in the future for the compilation purposes.<br>
However, I can temporarily place it under #if
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<span class="new">INCLUDE_JVMTI</span><br>
until the compiler team decides what to do with it.<br>
<br>
<blockquote cite="mid:5159E95E.2030705@oracle.com" type="cite">
<br>
Also, I think you should consider a global table rather than
one per class. I think we discussed this and may have
decided that speed of adding these is important, but
depending on how many are actually used per class, there's
one pointer per class.<br>
</blockquote>
<br>
I thought we've reached a consensus and decided to add MNT
pointer<br>
per class as it is performance sensitive for indy calls.<br>
Anyway, before changing it to a hashtable I'd want to hear
from John and Christian first.<br>
</blockquote>
<br>
Definitely would like to hear from John and Christian. We did
discuss it but I wasn't totally convinced this field is more
important than the others that were going to be moved out of
Klass* to make it smaller. Some statistics for some sample
applications like eclipse would be great.<br>
</blockquote>
<br>
I'm not sure what statistics do you mean here.<br>
Do you mean a footprint or performance statistics?<br>
<br>
As I understand from our previous discussion there can be a more
general solution for the footprint issue.<br>
As such we could attack the footprint problem separately.<br>
</blockquote>
<br>
Yes, there are more general solutions to the footprint issue. One
would be moving these JVMTI fields and initializing them lazily and
there are other footprint improvements. One additional improvement
would be to remove this field which will be mostly zero and put this
in a hashtable and not allocate it per class.<br>
<br>
<blockquote cite="mid:515B0F17.5040803@oracle.com" type="cite"> For
instance, we could separate all JVMTI fields from InstanceKlass
and initialize them lazily.<br>
But I'd not want to go deep into this discussion.<br>
You do not want me to get involved into the footprint work, do
you? :)<br>
<br>
Also, the decision how to represent the MNT depends on its future
usage by the compiler team.<br>
As we agreed, the compiler team is going to adjust the MNT to
their needs<br>
at some point when it is more convenient for them. <br>
So that, could we make a final decision when the whole picture is
ready?<br>
It would be better to approach it in some steps.<br>
Currently, this bug blocks other work on the JVMTI support of
jsr-292.<br>
</blockquote>
<br>
I don't know what the jsr 292 team has in store for this field but
it's still a footprint cost that's for only a special case. So this
is okay if you file a bug so that we can remove it and reimplement
this table to be global or a hashtable.<br>
<br>
Coleen<br>
<br>
<blockquote cite="mid:515B0F17.5040803@oracle.com" type="cite"> <br>
Thanks,<br>
Serguei<br>
<br>
<blockquote cite="mid:515A0F60.1040105@oracle.com" type="cite"> <br>
Coleen<br>
<br>
<blockquote cite="mid:515A03FB.30808@oracle.com" type="cite"> <br>
<blockquote cite="mid:5159E95E.2030705@oracle.com" type="cite">
Maybe they could be organized as a hashtable instead like
the method handle intrinsics?<br>
<br>
The code itself looks good everywhere, except for the
concern about footprint.<br>
</blockquote>
<br>
I very appreciate you found a time to review it!<br>
There are not many people having an expertize in this area.<br>
<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<blockquote cite="mid:5159E95E.2030705@oracle.com" type="cite">
<br>
Thanks,<br>
Coleen<br>
<br>
Can you make this mtClass on line <br>
<div class="moz-cite-prefix">On 03/26/2013 04:37 AM, <a
moz-do-not-send="true" class="moz-txt-link-abbreviated"
href="mailto:serguei.spitsyn@oracle.com">serguei.spitsyn@oracle.com</a>
wrote:<br>
</div>
<blockquote cite="mid:51515E3C.7040307@oracle.com"
type="cite">
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<br>
Please, review the following fix.<br>
The fix was preliminarily reviewed by Coleen, Christian<br>
and John but still a final and open jdk review is needed.<br>
So that, everyone is welcome to review the fix!<br>
<div class="moz-forward-container"> <br>
This is open webrev:<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Esspitsyn/webrevs/2013/hotspot/8008511-JVMTI-JSR292.1/">http://cr.openjdk.java.net/~sspitsyn/webrevs/2013/hotspot/8008511-JVMTI-JSR292.1/</a><br>
<br>
The CR is:<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8008511">http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8008511</a><br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://jbs.oracle.com/bugs/browse/JDK-8008511">https://jbs.oracle.com/bugs/browse/JDK-8008511</a><br>
<br>
<br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
The problem is that the old version of the bootstrap
method is re-invoked<br>
after a popframe from the bootstrap method execution.<br>
It is because the MemberName keeps a stale reference to
the old method version.<br>
<br>
The solution (suggested by John Rose) is to lazily
create and keep up-to-date a MemberNameTable<br>
which plays a role of MemberName cache assosiated with
the InstanceKlass.<br>
Then, at the Class redefinition, this cache is used to
replace the old method<br>
references in the MemberName's with the new method
references.<br>
<br>
The MemberNameTable is based on the
GrowableArray<jweek>.<br>
A C_HEAP array is allocated at the first call to <span
class="new">InstanceKlass::add_member_name</span>().<br>
It is released in the
InstanceKlass::release_C_heap_structures().<br>
<br>
A global week reference to member name oop is stored in
the MemberNameTable.<br>
It allowed to avoid having the oops_do() in the
MemberNameTable.<br>
Also, the MemberNameTable won't hold member name oops in
memory.<br>
<br>
The MemberNameTable_lock mutex is added to serialize
MemberNameTable's updates.<br>
<br>
The following No_Safepoint_Verifier has been disabled:<br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<b> src/share/vm/prims/methodHandles.cpp</b>:<br>
<meta http-equiv="content-type" content="text/html;
charset=windows-1252">
<pre> 799 int MethodHandles::find_MemberNames(Klass* k,
. . .
<span class="changed"> 803 DEBUG_ONLY(No_Safepoint_Verifier nsv);</span></pre>
It is probably Ok, but, please, let me know if it is
not.<br>
The assert related to locking is fired if it is not
disabled.<br>
<br>
Test coverage: vm.mlvm, nsk.jvmti, nsk.jdi tests on
multiple platforms (32 vs 64-bit too).<br>
The testing looks good so far.<br>
<br>
<br>
Thanks,<br>
Serguei<br>
<br>
</div>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>