FilePermission Canonical path optimization

[Peter Levart]

Hi Deven,

Similar to lazy initialization of initial list of drivers in 
java.sql.DriverManager that was done recently:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-December/029944.html

It would be nice to replace the following pattern:

if (cpath == null) initCannonicalpath();

... use cpath ...

with:

... use getCpath() ...

where the getCpath() contains lazy initialization logic. Unfortunately 
initCannonicalPath() does not initialize only the 'cpath', but also 
dependent 'directory' and 'recursive' flags, so perhaps you could rename 
it to initCpathDirectoryAndRecursive() ?

Your pattern:

if (cpath == null) initCannonicalpath();

... use cpath ...

also contains data races. The 'cpath', 'directory' and 'recursive' 
instance variables are not volatile. You set them in synchronized 
initCannonicalPath(), but you read them without synchronization. Another 
thread could see the 'cpath' variable already initialized, but still 
read the 'directory' and 'recursive' flags as uninitialized values (in 
equals and impliesIgnoreMask). These data races were present before, 
when those fields were initialized in constructor, but with lazy 
initialization you extend the possibilities of exposing them in 
situations to which they were not subjected before.

One way to fix this would be:

Make 'cpath', 'directory' and 'recursive' fields volatile and carefully 
arrange ininitCpathDirectoryAndRecursive () to be assigned in the 
following order:

- 'directory' and 'recursive' first
- 'cpath' last

The following pattern:

if (cpath == null) initCpathDirectoryAndRecursive();
... use 'cpath' or ''directory' or 'recursive' ...

Is then safe. And you don't need synchronized keyword on 
initCpathDirectoryAndRecursive().


Other than that, I think that the following check:

  191         if (getName() == null)
  192                 throw new NullPointerException("name can't be null");


...does not belong to initMask() method. But it has to be performed 
non-lazily in constructors / deserialization. So I suggest the following:

private static int checkMask(int mask) {
     if ((mask & ALL) != mask || mask == NONE)
         throw new IllegalArgumentException("invalid actions mask");

     return mask;
}

private static String checkPath(String path) {
     if (path == null)
         throw new NullPointerException("path can't be null");

     return path;
}

and use those in both constructors and deserialization as identity 
functions that pass the parameters unchanged, but validate them.


The question is what to do with the remaining data race that was present 
before. The 'mask' field. The best would be to make it final, but 
deserialization needs to set it. If you're willing to introduce ugly 
reflection with doPrivileged() monster block of code just to set the 
field in readObject(), you can do it like that:

// instead of

         this.mask = checkMask(getMask(actions));

// you would have to do do:

         final int mask = checkMask(getMask(actions));
         try {
             AccessController.doPrivileged(new 
PrivilegedExceptionAction<Void>() {
                 @Override
                 public Void run() throws Exception {
                     Field maskField = 
FilePermission.class.getDeclaredField("mask");
                     maskField.setAccessible(true);
                     maskField.set(FilePermission.this, mask);
                     return null;
                 }
             });
         } catch (PrivilegedActionException e) {
             throw new IOException(e.getException());
         }


Regards, Peter

On 12/05/2014 08:55 AM, deven you wrote:
> Hi All,
>
> I have updated the patch[3] to reflect all of your suggestions.
>
> [3] http://cr.openjdk.java.net/~youdwei/ojdk-912/webrev.02/
>
> Thanks a lot!
>
> 2014-12-05 10:39 GMT+08:00 deven you <ydwchina at gmail.com>:
>
>> Hi Alan,
>>
>> I am not clear whether canonicalization cache enabled or disabled, however
>> I think even the cache is enable, it may not help much for the start up
>> stage especially if the app uses many different files.
>>
>> The cache should speed up applications for common case especially after
>> start up and this patch will specifically solve the problem for
>> FilePermssion.
>>
>> I will update the patch soon with your suggestion and Wenjian's.
>>
>> Thanks a lot!
>>
>>
>>
>> 2014-12-01 20:50 GMT+08:00 Alan Bateman <Alan.Bateman at oracle.com>:
>>
>>> On 01/12/2014 08:06, deven you wrote:
>>>
>>>> Hi All,
>>>>    File.getCanonicalPath() is a very time-consuming method, we observed
>>>> significant performance degradation from some application's startup stage
>>>> with java.io.FilePermission. However, lazying load the calls to
>>>> getCanonicalPath() from java.ioFilePermission is straightforward and
>>>> solve
>>>> this problem effectively. Openjdk bug[1]  tracks this bug and here is the
>>>> patch [2]. Could anyone take a look?
>>>>
>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8066211
>>>> [2] http://cr.openjdk.java.net/~youdwei/ojdk-912/webrev.00/
>>>>
>>> Do you run with canonicalization cache enabled or disabled? While I don't
>>> agree with this cache, it seem to mostly eliminate complaints in this area
>>> after it was added (in JDK 1.4 I think).
>>>
>>> As regards the patch then it is a bit ugly. Could you use cpath == null
>>> instead of introducing a flag? If a flag is required then I think it will
>>> need a better name. Also if init is split then it might be cleaner to have
>>> initMask and initCanonicalPath.
>>>
>>> -Alan
>>>
>>>
>>>