[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR (M) 8213003: Remove more assignments in ifs for vmTestbase/[a-s]
From: JC Beyler <jcbeyler () google ! com>
Date: 2018-10-31 20:54:55
Message-ID: CAF9BGByaZGyjCCw6nZxW8eOo-6x6aw28HY033K29nSHTaXEKxg () mail ! gmail ! com
[Download RAW message or body]
@Claes: you are right, I did not do a grep such as "if.* =$"; by adding the
space instead of the $, I missed a few :)
I've been meaning to ask if we could deprecate these anyway. Are these
really being used? And if so, are we saying everything here is useful:
- Line/File + JNI call?
I ask because I'd like to deprecate the NSK_VERIFY macros but the LINE/FILE
is a bit annoying to deprecate.
I'd rather be able to remove them entirely but we could do an alternative
and migrate the NSK_VERIFY to:
testedFieldID = jni->GetStaticFieldID(testedClass, FIELD_NAME,
FIELD_SIGNATURE);
if (!testedFieldID) {
where the print out of the jni method and argument values can still be done
in the JNI wrapper I added (ExceptionCheckingJniEnv.cpp) so we'd have the
printout of the calls but not the line and filename from where the call was
done.
If lines and filenames are really important, then we could do something
like:
testedFieldID = NSK_TRACE(jni->GetStaticFieldID(testedClass,
FIELD_NAME, FIELD_SIGNATURE));
if (!testedFieldID) {
Which would add a line for which line/file is doing the JNI call. The
verify part can go into the wrapper I was talking about
(ExceptionCheckingJniEnv.cpp).
Thanks,
Jc
On Wed, Oct 31, 2018 at 11:52 AM Chris Plummer <chris.plummer@oracle.com>
wrote:
> On 10/31/18 11:30 AM, serguei.spitsyn@oracle.com wrote:
> > It prints the FILE/LINE which is enough to identify the error point.
> Yes, but it also prints the JNI calls.
> > But I agree that doing the assignments within the NSK_JNI_VERIFY was
> > intentional as it gives more context.
> > From the other hand it make the code ugly and less readable.
> > Not sure, what direction is better to go.
> Agreed. Somewhat of a tossup now as to whether or not this change should
> be done. I'm fine either way.
>
> Chris
> >
> > Thanks,
> > Serguei
> >
> >
> > On 10/31/18 11:21, Chris Plummer wrote:
> > > Hi Claes,
> > >
> > > It's good that you brought this up. NSK_JNI_VERIFY is always "on",
> > > but moving the assignment outside of it does slightly change the
> > > behavior of the macro (although the code still executes "correctly"):
> > >
> > > /**
> > > * Execute action with JNI call, check result for true and
> > > * pending exception and complain error if required.
> > > * Also trace action execution if tracing mode enabled.
> > > */
> > > #define NSK_JNI_VERIFY(jni, action) \
> > > (nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
> > > nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))
> > >
> > > So you will no longer see the JNI call in the trace or error output.
> > > You will just see the comparison to the JNI result, which gives no
> > > context as to the source of the result. So I'm now starting to think
> > > that doing the assignments within the NSK_JNI_VERIFY was intentional
> > > so we would see the JNI call in the trace output.
> > >
> > > Chris
> > >
> > > On 10/31/18 3:16 AM, Claes Redestad wrote:
> > > > Hi,
> > > >
> > > > here's a case that your script seems to miss:
> > > >
> > > >
> http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html
>
> > > >
> > > >
> > > > if (!NSK_JNI_VERIFY(jni, (testedFieldID =
> > > > jni->GetStaticFieldID(testedClass, FIELD_NAME,
> > > > FIELD_SIGNATURE)) != NULL))
> > > >
> > > > I'd note that with some of your changes here you're unconditionally
> > > > executing logic outside of NSK_JNI_VERIFY macros. Does care need be
> > > > taken to wrap the code blocks in equivalent macros/ifdefs? Or are
> > > > the NSK_JNI_VERIFY macros always-on in these tests (and essentially
> > > > pointless)?
> > > >
> > > > /Claes
> > > >
> > > > On 2018-10-31 05:42, JC Beyler wrote:
> > > > > Hi all,
> > > > >
> > > > > I worked on updating my script and handling more assignments so I
> > > > > redid a second pass on the same files to get all the cases. Now, on
> > > > > those files the regex "if.* = " no longer shows any cases we would
> > > > > want to fix.
> > > > >
> > > > > Could I get a review for this webrev:
> > > > > Webrev: http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/
> > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8213003
> > > > >
> > > > > I tested this on my dev machine by passing all the tests that were
> > > > > modified.
> > > > >
> > > > > Thanks!
> > > > > Jc
> > > >
> > >
> > >
> >
>
>
>
--
Thanks,
Jc
[Attachment #3 (text/html)]
<div dir="ltr"><div dir="ltr">@Claes: you are right, I did not do a grep such as \
"if.* =$"; by adding the space instead of the $, I missed a few \
:)</div><div dir="ltr"><br></div><div dir="ltr">I've been meaning to ask if we \
could deprecate these anyway. Are these really being used? And if so, are we saying \
everything here is useful:<div> - Line/File + JNI \
call?<br><div><br></div><div><div>I ask because I'd like to deprecate the \
NSK_VERIFY macros but the LINE/FILE is a bit annoying to \
deprecate.</div><div><br></div><div>I'd rather be able to remove them entirely \
but we could do an alternative and migrate the NSK_VERIFY \
to:</div><div><br></div><div> testedFieldID = \
jni->GetStaticFieldID(testedClass, FIELD_NAME, FIELD_SIGNATURE);</div><div> \
if (!testedFieldID) {<br></div><div><br></div><div>where the print out of the jni \
method and argument values can still be done in the JNI wrapper I added \
(ExceptionCheckingJniEnv.cpp) so we'd have the printout of the calls but not the \
line and filename from where the call was done.</div><div><br></div><div>If lines and \
filenames are really important, then we could do something like:</div><div><div> \
testedFieldID = NSK_TRACE(jni->GetStaticFieldID(testedClass, FIELD_NAME, \
FIELD_SIGNATURE));</div><div> if (!testedFieldID) {<br></div><br \
class="gmail-Apple-interchange-newline"></div><div>Which would add a line for which \
line/file is doing the JNI call. The verify part can go into the wrapper I was \
talking about (ExceptionCheckingJniEnv.cpp). \
</div><div><br></div><div><div>Thanks,<br></div></div><div>Jc</div><div><br></div></div></div></div></div><br><div \
class="gmail_quote"><div dir="ltr">On Wed, Oct 31, 2018 at 11:52 AM Chris Plummer \
<<a href="mailto:chris.plummer@oracle.com">chris.plummer@oracle.com</a>> \
wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">On 10/31/18 11:30 AM, <a \
href="mailto:serguei.spitsyn@oracle.com" \
target="_blank">serguei.spitsyn@oracle.com</a> wrote:<br> > It prints the \
FILE/LINE which is enough to identify the error point.<br> Yes, but it also prints \
the JNI calls.<br> > But I agree that doing the assignments within the \
NSK_JNI_VERIFY was <br> > intentional as it gives more context.<br>
> From the other hand it make the code ugly and less readable.<br>
> Not sure, what direction is better to go.<br>
Agreed. Somewhat of a tossup now as to whether or not this change should <br>
be done. I'm fine either way.<br>
<br>
Chris<br>
><br>
> Thanks,<br>
> Serguei<br>
><br>
><br>
> On 10/31/18 11:21, Chris Plummer wrote:<br>
>> Hi Claes,<br>
>><br>
>> It's good that you brought this up. NSK_JNI_VERIFY is always \
"on", <br> >> but moving the assignment outside of it does slightly \
change the <br> >> behavior of the macro (although the code still executes \
"correctly"):<br> >><br>
>> /**<br>
>> * Execute action with JNI call, check result for true and<br>
>> * pending exception and complain error if required.<br>
>> * Also trace action execution if tracing mode enabled.<br>
>> */<br>
>> #define NSK_JNI_VERIFY(jni, action) \<br>
>> (nsk_ltrace(NSK_TRACE_BEFORE,__FILE__,__LINE__,"%s\n",#action), \
\<br> >> nsk_jni_lverify(NSK_TRUE,jni,action,__FILE__,__LINE__,"%s\n",#action))<br>
>><br>
>> So you will no longer see the JNI call in the trace or error output. <br>
>> You will just see the comparison to the JNI result, which gives no <br>
>> context as to the source of the result. So I'm now starting to think \
<br> >> that doing the assignments within the NSK_JNI_VERIFY was intentional \
<br> >> so we would see the JNI call in the trace output.<br>
>><br>
>> Chris<br>
>><br>
>> On 10/31/18 3:16 AM, Claes Redestad wrote:<br>
>>> Hi,<br>
>>><br>
>>> here's a case that your script seems to miss:<br>
>>><br>
>>> <a href="http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev.00/test/ho \
tspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html" \
rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Ejcbeyler/8213003/webrev \
.00/test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassLoaderClasses/clsldrclss002/clsldrclss002.cpp.udiff.html</a> \
<br> >>><br>
>>><br>
>>> if (!NSK_JNI_VERIFY(jni, (testedFieldID =<br>
>>> jni->GetStaticFieldID(testedClass, \
FIELD_NAME, <br> >>> FIELD_SIGNATURE)) != NULL))<br>
>>><br>
>>> I'd note that with some of your changes here you're \
unconditionally <br> >>> executing logic outside of NSK_JNI_VERIFY macros. \
Does care need be <br> >>> taken to wrap the code blocks in equivalent \
macros/ifdefs? Or are <br> >>> the NSK_JNI_VERIFY macros always-on in these \
tests (and essentially <br> >>> pointless)?<br>
>>><br>
>>> /Claes<br>
>>><br>
>>> On 2018-10-31 05:42, JC Beyler wrote:<br>
>>>> Hi all,<br>
>>>><br>
>>>> I worked on updating my script and handling more assignments so I \
<br> >>>> redid a second pass on the same files to get all the cases. \
Now, on <br> >>>> those files the regex "if.* = " no longer \
shows any cases we would <br> >>>> want to fix.<br>
>>>><br>
>>>> Could I get a review for this webrev:<br>
>>>> Webrev: <a \
href="http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/" rel="noreferrer" \
target="_blank">http://cr.openjdk.java.net/~jcbeyler/8213003/webrev.00/</a><br> \
>>>> Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8213003" \
rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8213003</a><br>
>>>><br>
>>>> I tested this on my dev machine by passing all the tests that were \
<br> >>>> modified.<br>
>>>><br>
>>>> Thanks!<br>
>>>> Jc<br>
>>><br>
>><br>
>><br>
><br>
<br>
<br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" \
class="gmail_signature" data-smartmail="gmail_signature"><div \
dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic