[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] RFR: 8172813 test/java/awt/font/JNICheck/JNICheck.sh fails on Linux
From:       Prahalad Kumar Narayanan <prahalad.kumar.narayanan () oracle ! com>
Date:       2017-01-19 2:36:58
Message-ID: 56ef3390-831e-4183-a7ea-3370cd644f80 () default
[Download RAW message or body]

Hello Phil

Thanks for the detailed follow-up. The change looks good.

Btw, a small correction in the webrev link. ( , has been replaced with . )
http://cr.openjdk.java.net/~prr/8172813.2

Thanks
Have a good day

Prahalad N.


-----Original Message-----
From: Phil Race 
Sent: Thursday, January 19, 2017 5:36 AM
To: Prahalad Kumar Narayanan; 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] RFR: 8172813 test/java/awt/font/JNICheck/JNICheck.sh \
fails on Linux

A couple of updates made : http://cr.openjdk.java.net/~prr/8172813,2

Comments in-line below

-phil.



On 01/18/2017 09:51 AM, Prahalad Kumar Narayanan wrote:
> Hello Phil
> 
> The change looks good - Reduces the number of local references held by the JNI \
> function. Here are some findings :
> 
> 1.  I found additional references that could be released. You can check the \
>                 severity of the problem and decide to release or skip.
> Line: 1129    Concerned Object: cacheDirArray.
> I believe the existing code may not create trouble as reference to object is \
> outside the loop.

This is not really any different than the hundreds of other JNI methods that use some \
small number of local refs. It is indeed the ones in a loop that matter here.
> 
> Line: 1168 and
> Line: 1175     Concerned Object: fcCompFontObj
> Both these lines, either terminate /or continue the loop without releasing the \
> reference.

1168 I updated but 1175 causes a return to Java so freeing happens then \
automatically.
> 
> Line: 1330     Concerned Object: fcFont
> When jstr becomes NULL, for loop is exited 
> with break without releasing the reference

I looked at these before the first webrev and decided they weren't a problem in \
practice since this really should never happen.
> 
> Line: 1364    Concerned Object: fcFontArr
> fcFontArr (new object array ) is created in every iteration of the loop.
> This could be released in this line alongside 
> release of fcCompFontObj

I added a Delete here - line 1368 in the new version.
> 
> 2. Copyright notice to have 2017 instead of 2016

I rarely-to-never bother to update these. A script will be run at some point.

> 
> 3. Question: Should we mask the other warning messages with 'grep -v' in \
> JNICheck.sh ?

I am not sure what you are asking here.
I am masking the other warnings with grep -v .. and since I added doing that \
obviously I think we should :-)

-phil.

> 
> Thanks
> Have a good day
> 
> Prahalad N.
> 
> ------------------------------
> 
> Message: 2
> Date: Tue, 17 Jan 2017 12:29:24 -0800
> From: Phil Race <philip.race@oracle.com>
> To: Sergey Bylokhov <sergey.bylokhov@oracle.com>
> Cc: 2d-dev <2d-dev@openjdk.java.net>
> Subject: Re: [OpenJDK 2D-Dev] RFR: 8172813
> 	test/java/awt/font/JNICheck/JNICheck.sh fails on Linux
> Message-ID: <65d232e2-47e4-2a78-c454-c144876af564@oracle.com>
> Content-Type: text/plain; charset=windows-1252; format=flowed
> 
> Both yes and no :-), although I think it is fine to add both.
> 
> 1140 .. there are no more than 2 cachedirs (1 user + 1 system) so I 
> didn't examine JNI refs in this loop
> 
> 1166 .. I was not realistically expecting a "NULL" here so ignored it.
> 
> Unlike some of the JNI bugs "call while pending exception" type bugs we've fixed \
> these were actual, not hypothetical. 
> Updated webrev:
> 
> http://cr.openjdk.java.net/~prr/8172813.1
> 
> 
> -phil.
> 
> 
> On 01/16/2017 09:07 AM, Sergey Bylokhov wrote:
> > Hi, Phil.
> > Should we add DeleteLocalRef in fontpath.c at lines 1140 and 1166, or both were \
> > skipped intentionally? 
> > > Bug : https://bugs.openjdk.java.net/browse/JDK-8172813
> > > Webrev : http://cr.openjdk.java.net/~prr/8172813/
> > > 
> > > - Clear local refs once we are done with them.
> > > - Use grep to filter out from the log unrelated warnings as separately reported \
> > > here : 
> > > https://bugs.openjdk.java.net/browse/JDK-8131136
> > > 
> > > -phil.


[prev in list] [next in list] [prev in thread] [next in thread]