[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR(XS) 8193577: nsk/jvmti/IterateThroughHeap/filter-tagged fails with Graal in Xcomp mode
From: Igor Veresov <igor.veresov () oracle ! com>
Date: 2018-11-29 16:13:20
Message-ID: 7156C1B9-5151-4B4F-88AF-68F34D7BEFED () oracle ! com
[Download RAW message or body]
Thanks Dean and Serguei!
igor
> On Nov 29, 2018, at 8:00 AM, serguei.spitsyn@oracle.com wrote:
>
> Hi Igor,
>
> +1
>
> Thanks,
> Serguei
>
>
> On 11/29/18 07:42, dean.long@oracle.com <mailto:dean.long@oracle.com> wrote:
> > OK your fix looks good.
> >
> > dl
> >
> > On 11/28/18 10:57 PM, Igor Veresov wrote:
> > > It would work right now. But I don't want us fixing it again when somebody \
> > > implements effectively final optimization in Graal.
> > > igor
> > >
> > >
> > >
> > > > On Nov 28, 2018, at 10:02 PM, dean.long@oracle.com \
> > > > <mailto:dean.long@oracle.com> wrote:
> > > > I like it better. But do you really need to use JNI to reset the values? If \
> > > > you had
> > > > static int POISON = 0x1234; // not final
> > > >
> > > > class TaggedClass {
> > > > static int field1 = 0xC0DE01 + POISON;
> > > >
> > > > in HeapFilter.java, is the compiler smart enough to treat the value as \
> > > > constant until it changes?
> > > > dl
> > > >
> > > > On 11/28/18 9:26 PM, Igor Veresov wrote:
> > > > > Alright, how about the following solution: \
> > > > > http://cr.openjdk.java.net/~iveresov/8193577/webrev.01/ \
> > > > > <http://cr.openjdk.java.net/%7Eiveresov/8193577/webrev.01/>
> > > > > igor
> > > > >
> > > > >
> > > > >
> > > > > > On Nov 28, 2018, at 4:59 PM, Igor Veresov <igor.veresov@oracle.com \
> > > > > > <mailto:igor.veresov@oracle.com>> wrote:
> > > > > > I don't want to make it Graal specific. I think I'll just do field \
> > > > > > assignments in native so that it's all invisible to the compiler.
> > > > > > igor
> > > > > >
> > > > > >
> > > > > >
> > > > > > > On Nov 28, 2018, at 3:25 PM, serguei.spitsyn@oracle.com \
> > > > > > > <mailto:serguei.spitsyn@oracle.com> wrote:
> > > > > > > On 11/28/18 15:16, dean.long@oracle.com <mailto:dean.long@oracle.com> \
> > > > > > > wrote:
> > > > > > > > It sounds like the test could also fail with C2 if the fields are in \
> > > > > > > > a virtual object that was eliminated. I'm OK with your fix, but I \
> > > > > > > > would feel a little better if we only relaxed the check for Graal. I \
> > > > > > > > guess you'd need to use the whitebox api for that.
> > > > > > >
> > > > > > > I was thinking about the same.
> > > > > > > Relaxing this just for Graal sounds good.
> > > > > > > On the other hand, making the test to know about Graal looks a little \
> > > > > > > bit strange. :)
> > > > > > > Thanks,
> > > > > > > Serguei
> > > > > > >
> > > > > > > >
> > > > > > > > dl
> > > > > > > >
> > > > > > > > On 11/28/18 2:28 PM, Igor Veresov wrote:
> > > > > > > > > Oh, I haven't understood your idea before pressing reply. Yes, we \
> > > > > > > > > can match the objects by matching their shape, but it's also not an \
> > > > > > > > > exact solution prone to erroneous matches. Especially considering \
> > > > > > > > > the iteration API does callbacks for the fields out of order - it \
> > > > > > > > > does primitives, strings, arrays, in that order.
> > > > > > > > > There are also ways to make it fail with Graal that are not related \
> > > > > > > > > to constant representation. Graal treats allocations as side effect \
> > > > > > > > > free. So it's possible to allocate something and then deopt to a \
> > > > > > > > > point before the allocation and redo the allocation in the \
> > > > > > > > > interpreter. In this case there are going to be multiple objects on \
> > > > > > > > > the heap with only one of them being reachable.
> > > > > > > > > igor
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > On Nov 28, 2018, at 2:08 PM, Igor Veresov \
> > > > > > > > > > <igor.veresov@oracle.com <mailto:igor.veresov@oracle.com>> wrote: \
> > > > > > > > > > I don't think there is a way to identify an untagged object. \
> > > > > > > > > > There is nothing that is passed in the callback to allow that.
> > > > > > > > > > igor
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > On Nov 28, 2018, at 1:32 PM, dean.long@oracle.com \
> > > > > > > > > > > <mailto:dean.long@oracle.com> wrote:
> > > > > > > > > > > I'm guessing Graal creates Constant boxes of the individual \
> > > > > > > > > > > fields and not of the test objects? If so, can't we fix the \
> > > > > > > > > > > matching so that it checks that all fields of an object match? \
> > > > > > > > > > > I guess that would mean moving the "expected" and "found" \
> > > > > > > > > > > counts up from fields[] to objects_info[].
> > > > > > > > > > > dl
> > > > > > > > > > >
> > > > > > > > > > > On 11/28/18 1:13 PM, Igor Veresov wrote:
> > > > > > > > > > > > When doing heap iteration with JVMTI the way the object \
> > > > > > > > > > > > identity is tracked is by tagging. This particular test \
> > > > > > > > > > > > checks if it can observe an untagged object. Since there is \
> > > > > > > > > > > > no way to truly track the identity of an untagged object the \
> > > > > > > > > > > > test validates the identity by checking the value of the \
> > > > > > > > > > > > fields of such object. When being compiled with Graal there \
> > > > > > > > > > > > are objects produced (such as Constant boxes) that have field \
> > > > > > > > > > > > values that are equal to the expected values for the fields \
> > > > > > > > > > > > in UntaggedClass. During the subsequent heap iteration these \
> > > > > > > > > > > > objects are reported to the test and are treated as if they \
> > > > > > > > > > > > were instances of UntaggedClass.
> > > > > > > > > > > > The fix is to relax the test requirement and allow (for the \
> > > > > > > > > > > > untagged case) the number of the object found during \
> > > > > > > > > > > > iteration to be more than \
> > > > > > > > > > > > expected.
> > > > > > > > > > > > Webrev: \
> > > > > > > > > > > > http://cr.openjdk.java.net/~iveresov/8193577/webrev.00/ \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > \
> > > > > > > > > > > > JBS: https://bugs.openjdk.java.net/browse/JDK-8193577 \
> > > > > > > > > > > > <https://bugs.openjdk.java.net/browse/JDK-8193577>
> > > > > > > > > > > > igor
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
[Attachment #3 (unknown)]
<html><head><meta http-equiv="Content-Type" content="text/html; \
charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; \
line-break: after-white-space;" class=""><div class="">Thanks Dean and \
Serguei!</div><br class=""><div class=""> <span class="Apple-style-span" \
style="border-collapse: separate; color: rgb(0, 0, 0); font-family: Helvetica; \
font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: \
normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; \
text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; \
-webkit-border-horizontal-spacing: 0px; -webkit-border-vertical-spacing: 0px; \
-webkit-text-decorations-in-effect: none; -webkit-text-size-adjust: auto; \
-webkit-text-stroke-width: 0px; ">igor<br class=""><br class=""><br class=""></span>
</div>
<div><br class=""><blockquote type="cite" class=""><div class="">On Nov 29, 2018, at \
8:00 AM, <a href="mailto:serguei.spitsyn@oracle.com" \
class="">serguei.spitsyn@oracle.com</a> wrote:</div><br \
class="Apple-interchange-newline"><div class="">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" class="">
<div text="#000000" bgcolor="#FFFFFF" class="">
<div class="moz-cite-prefix">Hi Igor,<br class="">
<br class="">
+1<br class="">
<br class="">
Thanks,<br class="">
Serguei<br class="">
<br class="">
<br class="">
On 11/29/18 07:42, <a class="moz-txt-link-abbreviated" \
href="mailto:dean.long@oracle.com">dean.long@oracle.com</a> wrote:<br class=""> \
</div> <blockquote type="cite" \
cite="mid:99965d21-854d-eaf7-8de6-272defaff5e0@oracle.com" class=""> OK your fix \
looks good.<br class=""> <br class="">
dl<br class="">
<br class="">
<div class="moz-cite-prefix">On 11/28/18 10:57 PM, Igor Veresov
wrote:<br class="">
</div>
<blockquote type="cite" \
cite="mid:A661D7FE-AE79-4D25-BE73-6C3AB82E112B@oracle.com" class=""> <div \
class="">It would work right now. But I don't want us fixing it again when somebody \
implements effectively final optimization in Graal.</div>
<br class="">
<div class=""> <span class="Apple-style-span">igor<br class="">
<br class="">
<br class="">
</span> </div>
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">On Nov 28, 2018, at 10:02 PM, <a \
href="mailto:dean.long@oracle.com" class="" \
moz-do-not-send="true">dean.long@oracle.com</a> wrote:</div> <br \
class="Apple-interchange-newline"> <div class="">
<div class=""> I like it better. But do you really need
to use JNI to reset the values? If you had<br class="">
<br class="">
<tt class="">static int POISON = 0x1234; // not final</tt><tt \
class=""><br class=""> </tt><tt class=""><br class="">
</tt><tt class="">class TaggedClass {</tt><tt class=""><br class="">
</tt><tt class=""> static int field1 = \
0xC0DE01 + POISON;</tt><br class="">
<br class="">
in HeapFilter.java, is the compiler smart enough to
treat the value as constant until it changes?<br class="">
<br class="">
dl<br class="">
<br class="">
<div class="moz-cite-prefix">On 11/28/18 9:26 PM, Igor
Veresov wrote:<br class="">
</div>
<blockquote type="cite" \
cite="mid:9D54BD55-2C2D-40DF-96BD-BDB0EB5CD4CC@oracle.com" class=""> <div \
class="">Alright, how about the following solution: <a \
href="http://cr.openjdk.java.net/%7Eiveresov/8193577/webrev.01/" class="" \
moz-do-not-send="true">http://cr.openjdk.java.net/~iveresov/8193577/webrev.01/</a></div>
<div class=""><br class="">
</div>
<div class=""> <span class="Apple-style-span">igor<br class="">
<br class="">
<br class="">
</span> </div>
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">On Nov 28, 2018, at 4:59 PM, Igor
Veresov <<a href="mailto:igor.veresov@oracle.com" class="" \
moz-do-not-send="true">igor.veresov@oracle.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div class="">I don't want to make it Graal
specific. I think I'll just do field
assignments in native so that it's all
invisible to the compiler.
<div class=""><br class="">
<div class=""> <span class="Apple-style-span">igor<br \
class=""> <br class="">
<br class="">
</span> </div>
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">On Nov 28, 2018, at 3:25
PM, <a href="mailto:serguei.spitsyn@oracle.com" \
class="" moz-do-not-send="true">serguei.spitsyn@oracle.com</a> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div class="">
<div class="moz-cite-prefix">On
11/28/18 15:16, <a \
class="moz-txt-link-abbreviated" href="mailto:dean.long@oracle.com" \
moz-do-not-send="true">dean.long@oracle.com</a> wrote:<br class="">
</div>
<blockquote type="cite" \
cite="mid:e97be0a0-89ca-1a1e-3c4f-77dcaba14230@oracle.com" class=""> It sounds like \
the test could also fail with C2 if the
fields are in a virtual object
that was eliminated. I'm OK with
your fix, but I would feel a
little better if we only relaxed
the check for Graal. I guess
you'd need to use the whitebox api
for that.<br class="">
</blockquote>
<br class="">
I was thinking about the same.<br class="">
Relaxing this just for Graal sounds
good.<br class="">
On the other hand, making the test
to know about Graal looks a little
bit strange. :)<br class="">
<br class="">
Thanks,<br class="">
Serguei <br class="">
<br class="">
<blockquote type="cite" \
cite="mid:e97be0a0-89ca-1a1e-3c4f-77dcaba14230@oracle.com" class=""> <br class=""> \
dl<br class=""> <br class="">
<div class="moz-cite-prefix">On
11/28/18 2:28 PM, Igor Veresov
wrote:<br class="">
</div>
<blockquote type="cite" \
cite="mid:923D4C45-2684-41F6-9CD7-6C8D7BABBA5C@oracle.com" class=""> <div \
class="">Oh, I haven't understood your idea before
pressing reply. Yes, we can
match the objects by matching
their shape, but it's also not
an exact solution prone to
erroneous matches. Especially
considering the iteration API
does callbacks for the fields
out of order - it does
primitives, strings, arrays,
in that order.</div>
<div class=""><br class="">
</div>
<div class="">There are also
ways to make it fail with
Graal that are not related to
constant representation. Graal
treats allocations as side
effect free. So it's possible
to allocate something and then
deopt to a point before the
allocation and redo the
allocation in the interpreter.
In this case there are going
to be multiple objects on the
heap with only one of them
being reachable.</div>
<div class=""><br class="">
</div>
<div class=""> <span \
class="Apple-style-span">igor<br class=""> <br class="">
<br class="">
</span> </div>
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">On Nov 28,
2018, at 2:08 PM, Igor
Veresov <<a \
href="mailto:igor.veresov@oracle.com" class="" \
moz-do-not-send="true">igor.veresov@oracle.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div class="">
<div class="">I don't
think there is a way
to identify an
untagged object. There
is nothing that is
passed in the callback
to allow that.</div>
<br class="">
<div class=""> <span \
class="Apple-style-span">igor<br class=""> <br class="">
</span> </div>
<div class=""><br class="">
<blockquote type="cite" class="">
<div class="">On Nov
28, 2018, at 1:32
PM, <a \
href="mailto:dean.long@oracle.com" class="" \
moz-do-not-send="true">dean.long@oracle.com</a> wrote:</div>
<br \
class="Apple-interchange-newline"> <div class="">
<div class="">I'm
guessing Graal
creates Constant
boxes of the
individual
fields and not
of the test
objects? If so,
can't we fix the
matching so that
it checks that
all fields of an
object match? I
guess that would
mean moving the
"expected" and
"found" counts
up from fields[]
to
objects_info[].<br class="">
<br class="">
dl<br class="">
<br class="">
On 11/28/18 1:13
PM, Igor Veresov
wrote:<br class="">
<blockquote type="cite" \
class="">When doing heap
iteration with
JVMTI the way
the object
identity is
tracked is by
tagging. This
particular
test checks if
it can
observe an
untagged
object. Since
there is no
way to truly
track the
identity of an
untagged
object the
test validates
the identity
by checking
the value of
the fields of
such object.
When being
compiled with
Graal there
are objects
produced (such
as Constant
boxes) that
have field
values that
are equal to
the expected
values for the
fields in
UntaggedClass.
During the
subsequent
heap iteration
these objects
are reported
to the test
and are
treated as if
they were
instances of
UntaggedClass.<br class="">
<br class="">
The fix is to
relax the test
requirement
and allow (for
the untagged
case) the
number of the
object found
during
iteration to
be more than
expected.<br class="">
<br class="">
Webrev: <a \
href="http://cr.openjdk.java.net/%7Eiveresov/8193577/webrev.00/" class="" \
moz-do-not-send="true">http://cr.openjdk.java.net/~iveresov/8193577/webrev.00/</a><br \
class=""> JBS: <a href="https://bugs.openjdk.java.net/browse/JDK-8193577" class="" \
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8193577</a><br \
class=""> <br class="">
igor<br class="">
<br class="">
<br class="">
<br class="">
</blockquote>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</blockquote>
<br class="">
</blockquote>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</div>
</div>
</blockquote>
</div>
<br class="">
</blockquote>
<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</blockquote>
<br class="">
</blockquote>
<br class="">
</div>
</div></blockquote></div><br class=""></body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic