Remi, Thanks for the review and comments, you have some good points.
I am thinking perhaps the dumper class could be used somewhere else given a different path, which is why the directory is used as key to get an instance and injected from caller. As you observed, it's not necessary. But the injection does makes a cleaner separation instead of having nasty circle dependency as you put in the comment. I do intentionally leave the check on dumpDir each time instead of only in static block so that if the directory specified is not setup correctly(i.e, not exist), it can be fixed easily. I am thinking about debugging long-live process, but that's probably a moo point. As the static string for property name, we can simply hard-code it than we won't need static block nor a local field. This is just following a general convention in case somewhere else such as command line help need to have access to that constant. Sometimes I have difficulty to trade-off simplicity and extensibility, although I know the principal should be probably "Don't over engineer until you need it", but it's hard to resist the low-hanging fruit. :) Not sure how strong you feel about this, I'll let it sit for others to comment on current code/approach before I start another revision if necessary. Cheers, Henry On Oct 1, 2013, at 4:54 PM, Remi Forax <fo...@univ-mlv.fr> wrote: > On 10/02/2013 01:04 AM, Henry Jen wrote: >> Hi, >> >> Please review the updated webrev at >> http://cr.openjdk.java.net/~henryjen/ccc/8023524/3/webrev/ >> >> This update addressed comments from Mandy with following, >> >> - call doPrivileged with specific file permission, "<<ALL FILES>>", "write". >> - Use nio package to write deal with FS, also create directory hierarchy >> with package name instead of flat files. >> - If not specify a value or empty string, the directory is default to >> current working directory as "." is specified. >> >> We continue to use local encoding rule as other suggestions doesn't fit. >> The reason we cannot use something like URLEncoder is that ":" won't get >> encoded and it's not valid character for filename on Windows. All the >> suggestions so far seems to have that same issue. >> >> Cheers, >> Henry >> > > Hi Henry, > most of the code that use atomic values is not needed if you do all these > checks > in the static block (because the static block is protected by a lock) and > also instead of injecting the dump_dir, I think it's better to ask for it > in ProxyClassesDumper. > > so first, nitpicking!, DUMP_PATH_PROPERTY doesn't need to be a static field, > the code below is equivalent to yours but avoid to create yet another static > field. > > // For dumping generated classes to disk, for debugging purposes > /* package */ static final String LAMBDA_PROXY_DUMP_DIR; > static { > String DUMP_PATH_PROPERTY = "jdk.internal.lambda.dumpProxyClasses"; > LAMBDA_PROXY_DUMP_DIR= AccessController.doPrivileged(new > GetPropertyAction(DUMP_PATH_PROPERTY)); > } > > > // If requested, dump out to a file for debugging purposes > if (LAMBDA_PROXY_DUMP_DIR != null) { > AccessController.doPrivileged(new PrivilegedAction<Void>() { > @Override > public Void run() { > ProxyClassesDumper dumper = > ProxyClassesDumper.getInstance(); // dont send DUMP_DIR here > ... > > then as I said the code in ProxyClassesDumper can be simplified if instead > of sending the dump directory, the ProxyClassesDumper ask for it. > > final class ProxyClassesDumper { > ... > > private final static ProxyClassesDumper INSTANCE; > static { > // ProxyClassesDumper.getInstance() should never be called > // in the static block of InnerClassLambdaMetafactory > // because it will create a circular dependency (a NPE) > String pathname = > InnerClassLambdaMetafactory.LAMBDA_PROXY_DUMP_DIR.trim(); > Path dumpDir = Paths.get(pathname.isEmpty() ? "." : pathname); > dumpDir = validateDumpDir(dumpDir)? dumpDir: null; > INSTANCE = new ProxyClassesDumper(dumpDir); > } > private final Path dumpDir; // null if invalid > > // implement dump singleton instance, that's all we need right now > public static ProxyClassesDumper getInstance() { > return INSTANCE; > } > > private ProxyClassesDumper(String path) { > this.path = path; > } > > private static boolean validateDumpDir(Path path) { > String errMsg; // no need to initialize errMsg here > if (!Files.exists(path)) { > ... > public void dumpClass(String className, final byte[] classBytes) { > if (dumpDir == null) { > // invalid dump directory, silently ignore it > return; > } > ... > > cheers, > Rémi >
