On Wed, 21 Aug 2024 20:25:07 GMT, Chen Liang <[email protected]> wrote:
> @cl4es discovered that Stack Map generation in ClassFile API uses
> `componentType` and `arrayType` for `aaload` `aastore` instructions, which
> are currently quite slow. We can split out array class descriptors from class
> or interfaces to support faster `arrayType` and `componentType` operations.
>
> Tentative, as I currently have no way to measure the actual impact of this
> patch on the startup performance; however, this made the `ClassDesc`
> implementations much cleaner.
Nice cleanup! I'm just not really sure about the new subtype relationship (see
inline comment)
src/java.base/share/classes/jdk/internal/constant/ArrayClassDescImpl.java line
116:
> 114: sb.append(componentDesc);
> 115: return sb.toString();
> 116: }
Is there really that much benefit in lazily computing the descriptor?
`ReferenceClassDescImpl` doesn't do this either... Maybe we can keep things
simple and initialize the descriptor in the constructor?
src/java.base/share/classes/jdk/internal/constant/ArrayClassDescImpl.java line
126:
> 124: Class<?> clazz = element.resolveConstantDesc(lookup);
> 125: for (int i = 0; i < rank; i++)
> 126: clazz = clazz.arrayType();
Looking at the implementation of `arrayType`, it reflectively creates an array
and then returns its class. Just makes me think we need a better we to look up
an array class directly in the JDK :D
src/java.base/share/classes/jdk/internal/constant/MethodTypeDescImpl.java line
89:
> 87:
> 88: private static ClassDesc validateArgument(ClassDesc arg) {
> 89: if (requireNonNull(arg) == CD_void)
Is it safe to make this change? Are instances of `PrimitveClassDescImpl`
canonicalized now? (not in this patch, but it looks that way in the source
code).
src/java.base/share/classes/jdk/internal/constant/ReferenceClassDescImpl.java
line 58:
> 56: if (descriptor.charAt(0) == '[') {
> 57: return ArrayClassDescImpl.ofValidatedDescriptor(descriptor);
> 58: }
I think arrays should be handled separately by the caller, or
`ArrayClassDescImpl` should be made a sub-type of `ReferenceClassDescImpl`.
This factory in `ReferenceClassDescImpl` handling a sibling seems a bit
confusing.
-------------
PR Review: https://git.openjdk.org/jdk/pull/20665#pullrequestreview-2348740185
PR Review Comment: https://git.openjdk.org/jdk/pull/20665#discussion_r1788073751
PR Review Comment: https://git.openjdk.org/jdk/pull/20665#discussion_r1788084532
PR Review Comment: https://git.openjdk.org/jdk/pull/20665#discussion_r1788090312
PR Review Comment: https://git.openjdk.org/jdk/pull/20665#discussion_r1788110720
