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 <ydwch...@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.bate...@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
