RFR: 8304846: Provide a shared utility to dump generated classes defined via Lookup API [v5]

[Jorn Vernee]

On Thu, 30 Mar 2023 18:46:25 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>> This implements a shared utility to dump generated classes defined as normal/hidden classes via `Lookup` API.   This replaces the implementation in `LambdaMetaFactory` and method handle implementation that dumps the hidden class bytes on disk for debugging.   
>> 
>> For classes defined via `Lookup::defineClass`, `Lookup::defineHiddenClass` and `Lookup::defineHiddenClassWithClassData`, by default they will be dumped to the path specified in `-Djava.lang.invoke.Lookup.dumpClasses=<dumpDir>` 
>> 
>> The hidden classes generated for lambdas, `LambdaForms` and method handle implementation use non-default dumper so that they can be controlled via a separate system property and path as in the current implementation.
>> 
>> To dump lambda proxy classes, set this system property:
>>    -Djdk.internal.lambda.dumpProxyClasses=<dumpDir>
>> 
>> To dump LambdaForms and method handle implementation, set this system property:
>>    -Djava.lang.invoke.MethodHandle.DUMP_CLASS_FILES=true
>>    
>> P.S. `ProxyClassesDumper` is renamed to `ClassFileDumper` but for some reason, it's not shown as rename.
>
> Mandy Chung has updated the pull request incrementally with one additional commit since the last revision:
> 
>   set -D<key> or -D<key>=true will enable dumping

Looks mostly good.

src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java line 85:

> 83: 
> 84:     // Used to ensure that dumped class files for failed definitions have a unique class name
> 85:     private static final AtomicInteger counter = new AtomicInteger();

This counter could be removed now, it looks like.

src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 2536:

> 2534:                             }
> 2535:                         } else {
> 2536:                             name += ".failed-" + dumper.incrementAndGetCounter();

I think it makes more sense to move the counter to `ClassDefiner`. It is not used by `ClassFileDumper` itself.

(make it `static` if it's possible to have multiple definer instances with the same class name)

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java line 115:

> 113:         if (dumper.isEnabled() && !path.equals(dumper.dumpPath())) {
> 114:             throw new IllegalArgumentException("mismatched dump path for " + key);
> 115:         }

I don't see how this exception case could ever occur, given that `dumper.dumpPath` is directly coming from `dir`, and `validateDumpDir` doesn't return a modified path either.

Can this check be removed?

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java line 145:

> 143:     public Path pathname(String internalName) {
> 144:         return dumpDir.resolve(encodeForFilename(internalName) + ".class");
> 145:     }

Could be `private` I think

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java line 182:

> 180:                     if (!Files.exists(path)) {
> 181:                         try {
> 182:                             Files.createDirectory(path);

Maybe this could use `createDirectories` instead, in case the path is nested. `dumpClass` also uses that.

-------------

PR Review: https://git.openjdk.org/jdk/pull/13182#pullrequestreview-1369526948
PR Review Comment: https://git.openjdk.org/jdk/pull/13182#discussion_r1156289723
PR Review Comment: https://git.openjdk.org/jdk/pull/13182#discussion_r1156296104
PR Review Comment: https://git.openjdk.org/jdk/pull/13182#discussion_r1156281816
PR Review Comment: https://git.openjdk.org/jdk/pull/13182#discussion_r1156284973
PR Review Comment: https://git.openjdk.org/jdk/pull/13182#discussion_r1156280537