On 10/02/2013 07:42 AM, Henry Jen wrote:
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
I think there is no trade off here, the code is not public, so no headache.
We should only value the simplicity and refactor 'if somewhere else ...'.
There is a nice way to solve the circular issue, the idea is that you
can declare
a ProxyClassesDumper in InnerClassLambdaMetafactory and initialize it
with null
if the property is not defined. In that case, you don't need
ProxyClassesDumper to be
a singleton anymore and because the static field is initialized with null,
the class is not loaded if you don't want to dump the class file.
private static final ProxyClassesDumper PROXY_CLASSES_DUMPER;
static {
String DUMP_PATH_PROPERTY = "jdk.internal.lambda.dumpProxyClasses";
String dumpDir = AccessController.doPrivileged(new
GetPropertyAction(DUMP_PATH_PROPERTY));
PROXY_CLASSES_DUMPER = (dumpDir == null)? null:
ProxyClassesDumper.create(dumpDir);
}
// If requested, dump out to a file for debugging purposes
if (PROXY_CLASSES_DUMPER != null) {
AccessController.doPrivileged(new PrivilegedAction<Void>() {
@Override
public Void run() {
PROXY_CLASSES_DUMPER. ...
...
final class ProxyClassesDumper {
...
private final Path dumpDir; // null if invalid
public static ProxyClassesDumper create(String dir) {
dir = dir.trim();
Path path = Paths.get(dir.isEmpty() ? "." : dir);
path = validateDumpDir(path)? path: null;
return new ProxyClassesDumper(path);
}
private ProxyClassesDumper(Path 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
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
