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 <[email protected]>
To: Sergey Bylokhov <[email protected]>
Cc: 2d-dev <[email protected]>
Subject: Re: [OpenJDK 2D-Dev] RFR: 8172813
test/java/awt/font/JNICheck/JNICheck.sh fails on Linux
Message-ID: <[email protected]>
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.
