RFR (4th): 8023524: Mechanism to dump generated lambda classes / log lambda code generation

[Remi Forax]

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 <forax at 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
>>