On Fri, 4 Oct 2024 17:38:51 GMT, Jorn Vernee <jver...@openjdk.org> 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.
>
> 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?
This laziness is actually the main reason I had this implementation: In stack
map generation, we need to compute the array descriptors of many types yet not
using them in the end; the string computation involved a lot of allocations,
especially with frequent `arrayType()` and `componentType()` calls.
> 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
Indeed; and it's more alarming that `Class.forName("[[[[[Ljava.lang.Object;")`
is slower than
`Object.class.arrayType().arrayType().arrayType().arrayType().arrayType()`.
> 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).
Yes, they are canonicalized now for performance. `PrimitiveClassDescImpl`
extends `DynamicConstantDesc` so creation and comparison of non-unique
instances was costly.
> 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 type seems a bit
> confusing.
Indeed, since the factory usages are widespread, I only did a hotfix migration.
This class should now be called `ClassOrInterfaceDescImpl` (or "type klass" in
hotspot terms or "declared types" in javac terms)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20665#discussion_r1788128649
PR Review Comment: https://git.openjdk.org/jdk/pull/20665#discussion_r1788125022
PR Review Comment: https://git.openjdk.org/jdk/pull/20665#discussion_r1788126100
PR Review Comment: https://git.openjdk.org/jdk/pull/20665#discussion_r1788127459
