On Thu, 30 Mar 2023 18:46:25 GMT, Mandy Chung <mch...@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
