Re: RFR [9] 8040837: Avoid provoking NFEs when initializing InetAddrCachePolicy

[Michael McMahon]

I think it would be an improvement to combine these doPrivileged() blocks
as suggested, though your patch needs work Bernd. For instance,
the multi-catch doesn't work. Also the PrivilegedAction<> type is wrong.

If someone wants to update it, then we can use that. Otherwise, we'll
go with the original suggested change.

Thanks
Michael

On 17/04/14 21:28, Bernd Eckenfels wrote:

Am Thu, 17 Apr 2014 21:50:23 +0200
schrieb Bernd Eckenfels :


Hello,

I would propose to use Integer.valueOf(tmp) instead, but looking at
the context I think it is even better to skip this and the following
null check with Integer.parseInt().

This is even shorter and it reduces the privileged actions invocations:

Integer tmp = java.security.AccessController.doPrivileged (
new PrivilegedAction() {
public String run() {
try {
String tmpString = Security.getProperty(cachePolicyProp);
   if (tmpString != null) return Integer.valueOf(tmpString);
} catch (NumberFormatException | IllegalArgumentException 
ignored) { }

try {
return Integer.getInteger(cachePolicyPropFallback);
} catch (NumberFormatException | IllegalArgumentException 
ignored) { }

return null;
}
});

 if (tmp != null) {
 cachePolicy = tmp.intValue();
 if (cachePolicy < 0) {
 cachePolicy = FOREVER;
 }
 propertySet = true;
 } else {
 /* No properties defined for positive caching. If there is no
  * security manager then use the default positive cache value. */
 if (System.getSecurityManager() == null) {
 cachePolicy = DEFAULT_POSITIVE;
 }
 }

NB: this will

Re: RFR [9] 8040837: Avoid provoking NFEs when initializing InetAddrCachePolicy

[Mike Duigou]

Claes tidied things up to produce a workable patch:

> Here is the updated webrev:
> 
> http://cr.openjdk.java.net/~mduigou/JDK-8040837/0/webrev/
> 
> I will push it to jdk9/dev/jdk on Friday before COB for Claes unless I hear 
> objections.
> 
> Cheers,
> 
> Mike

On Apr 18 2014, at 09:27 , Michael McMahon  wrote:

> I think it would be an improvement to combine these doPrivileged() blocks
> as suggested, though your patch needs work Bernd. For instance,
> the multi-catch doesn't work. Also the PrivilegedAction<> type is wrong.
> 
> If someone wants to update it, then we can use that. Otherwise, we'll
> go with the original suggested change.
> 
> Thanks
> Michael
> 
> On 17/04/14 21:28, Bernd Eckenfels wrote:
>> Am Thu, 17 Apr 2014 21:50:23 +0200
>> schrieb Bernd Eckenfels :
>> 
>>> Hello,
>>> 
>>> I would propose to use Integer.valueOf(tmp) instead, but looking at
>>> the context I think it is even better to skip this and the following
>>> null check with Integer.parseInt().
>> This is even shorter and it reduces the privileged actions invocations:
>> 
>>Integer tmp = java.security.AccessController.doPrivileged (
>>new PrivilegedAction() {
>>public String run() {
>>try {
>>String tmpString = Security.getProperty(cachePolicyProp);
>> if (tmpString != null) return Integer.valueOf(tmpString);
>>} catch (NumberFormatException | IllegalArgumentException 
>> ignored) { }
>> 
>>try {
>>return Integer.getInteger(cachePolicyPropFallback);
>>} catch (NumberFormatException | IllegalArgumentException 
>> ignored) { }
>> 
>>return null;
>>}
>>});
>> 
>> if (tmp != null) {
>> cachePolicy = tmp.intValue();
>> if (cachePolicy < 0) {
>> cachePolicy = FOREVER;
>> }
>> propertySet = true;
>> } else {
>> /* No properties defined for positive caching. If there is no
>>  * security manager then use the default positive cache value. */
>> if (System.getSecurityManager() == null) {
>> cachePolicy = DEFAULT_POSITIVE;
>> }
>> }
>> 
>> NB: this will
> 

Re: RFR [9] 8040837: Avoid provoking NFEs when initializing InetAddrCachePolicy

[Bernd Eckenfels]

Am Fri, 18 Apr 2014 17:27:47 +0100
schrieb Michael McMahon :

> I think it would be an improvement to combine these doPrivileged()
> blocks as suggested, though your patch needs work Bernd. For instance,
> the multi-catch doesn't work. Also the PrivilegedAction<> type is
> wrong.

Yes I noticed those and sent them to Claes as he contacted me off-list,
I could provide a refreshed patch, I need to work on my build/test
environment however.

We should use Integer.decode() I think, that was wrong in below
suggestion.

BTW: one additional question: the name of the properties name fields
confused me as they look dynamic. Can this be changed into ALL_CAPS
constand names, as well?

Bernd

RFR [9] 8040747 : Improve performance of IP address parsing

[Claes Redestad]

Hi,

 could I get a review of the following patch to improve IP address 
parsing performance?


http://cr.openjdk.java.net/~mduigou/JDK-8040747/0/webrev/ 


https://bugs.openjdk.java.net/browse/JDK-8040747

 A set of simple JMH benchmarks was created to test all supported and 
some invalid formats. Results show a speed up of ~5-9x for the normal 
cases (9x for simple cases like 0.0.0.0, 5x for longer addresses like 
255.255.255.255), similar boosts for all invalid and rare cases while 
profiling show that the TLAB allocation rates drop significantly using 
the proposed algorithm.


 Note: naturally maintaining compatibility with the standard and 
non-standard formats intently supported by the previous implementation 
(where both 127.0.0.1 and 4000 would be considered valid IPv4 
addresses), but I've omitted support for the use of explicit signs 
(+127.+0.-0.+1 would work with the old parser) due to reliance on 
Integer.parseInt to parse the octets; a few checks could be added to 
keep supporting these unintended format variants at a small cost.


 /Claes