Hi Coleen,
Sorry, I've replied to the wrong email thread.
Okay, I'll look at correct webrev.
Thanks,
Serguei
On 12/17/19 18:09, [email protected] wrote:
Hi
Serguei,
The review thread should be "RFR 8235829: graal crashes with
Zombie.java test".
Hi
Coleen,
Is this webrev v2 right to look at? :
http://cr.openjdk.java.net/~coleenp/2019/8235829.02/webrev/
Actually, this one was something I tried that didn't work, so the
review was 01. I created 02 with the change to make
run_nmethod_entry_barriers plural:
open webrev at http://cr.openjdk.java.net/~coleenp/2019/8235829.02/webrev
Can you review this one on the other thread?
Thanks,
Coleen
It
looks good to me.
Just one nit (sorry, if it is a duplicated comment):
http://cr.openjdk.java.net/~coleenp/2019/8235829.02/webrev/src/hotspot/share/prims/jvmtiImpl.cpp.frames.html
1066 void JvmtiDeferredEventQueue::run_nmethod_entry_barrier() {
1067 for(QueueNode* node = _queue_head; node != NULL; node = node->next()) {
1068 node->event().run_nmethod_entry_barrier();
1069 }
1070 }
The function run_nmethod_entry_barrier()
should have a plural form as it iterates over the queue.
Thanks
Serguei
On 12/17/19 4:21 AM, Robbin Ehn wrote:
Hi Coleen,
On 12/16/19 9:21 PM, [email protected]
wrote:
http://cr.openjdk.java.net/~rehn/8235912/v1/webrev/src/hotspot/share/prims/jvmtiImpl.cpp.udiff.html
+ NativeAccess<>::oop_store(_class_holder,
class_holder_oop);
This should probably be:
41
NativeAccess<IS_DEST_UNINITIALIZED>::oop_store(handle,
obj);
I have not seen any stores to oopStorage that use that?
oopStorage should be 'initialized'.
So I prefer not adding another decorator if it's not needed.
That would just be confusing.
It's in the ClassLoaderData initialization. I don't know what
it means, you'd have to ask someone who knows. I don't think
it matters though.
Coleen
You can leave out using OopHandle. I have a patch to add
the missing functionality and add it to your code.
Actually, I was looking to see how much OopHandle is used
to see if it's helping anything and there is a lot of code
using it. Most of it is to hide oop* in ClassLoaderData.
This change otherwise looks great.
Thanks, Robbin
Thanks,
Coleen
Thanks for having a look, Robbin
On 12/16/19 1:32 PM, [email protected]
wrote:
I have to think about this. Could there be
breakpoints in old emcp methods that we do not
remove? The metadata_do function is trying to keep
old Methods from being deleted while there are still
references to them.
http://cr.openjdk.java.net/~rehn/8235912/v1/webrev/src/hotspot/share/prims/jvmtiImpl.hpp.udiff.html
+ oop* _class_holder; // keeps _method memory from
being deallocated
We created the class OopHandle to encapsulate strong
oopStorage references, although it's missing
oop_store. Can you use that?
Coleen
On 12/16/19 4:47 AM, Robbin Ehn wrote:
Hi all, please review.
From issue, https://bugs.openjdk.java.net/browse/JDK-8235912:
JvmtiBreakpoints are walked via VMThread oops_do
(the breakpoint is in a vm operation) before they
are installed in the safeopint and after they have
been installed, walked with
JvmtiCurrentBreakpoints::oops_do().
By putting the class holder inside oopStorage there
is no need for this.
JvmtiCurrentBreakpoints::metadata_do is not needed
because redefine classes actually removes the
breakpoints before updating them (so there is no
breakpoints to update).
We can just remove metadata_do.
I also removed some unused code.
Changeset:
http://cr.openjdk.java.net/~rehn/8235912/v1/webrev/
Passes several runs of nsk jvmti/jdi and t1-7.
Thanks, Robbin
|
