RFR: 8306842: Classfile API performance improvements

[Chen Liang]

On Wed, 26 Apr 2023 15:04:50 GMT, Adam Sotona <asotona at openjdk.org> wrote:

> Following improvements implemented:
> - Switch over `String` replaced with switch single char
> - Binary search for frames in `StackMapGenerator`
> - `StackMapGenerator.rawHandlers` with pre-calculated offsets
> - `ClassEntry` is caching `ClassDesc` symbol
> - Caching of type symbols in `NameAndTypeEntry` and `MethodTypeEntry`
> - Caching `MethodTypeDesc` in `MethodInfo` implementations
> - `StackMapGenerator` and `Utils` delegating to cached `MethodTypeDesc` instead of custom parsing
> 
> No API change.
> 
> Benchmarks show stack map generation improved by 21% and code generation from symbols improved by 30%.
> 
> 
> Benchmark                     Mode  Cnt       Score       Error  Units
> GenerateStackMaps.benchmark  thrpt   10  407931.202 ± 13101.023  ops/s
> RebuildMethodBodies.shared   thrpt    4   10258.597 ±   383.699  ops/s
> RebuildMethodBodies.unshared thrpt    4    7224.543 ±   256.800  ops/s
> 
> 
> 
> Benchmark                     Mode  Cnt       Score      Error  Units
> GenerateStackMaps.benchmark  thrpt   10  495101.110 ± 2389.628  ops/s
> RebuildMethodBodies.shared    thrpt   4   13380.272 ±  810.113  ops/s
> RebuildMethodBodies.unshared  thrpt   4    9399.863 ±  557.060  ops/s

1. CodeBuilder.invokeInstruction and CodeBuilder.fieldInstruction can pass their symbols to the NameAndTypeEntry as well, since it's almost always going to be used by stack map generation later.
2. Since the casts to access the field and method type symbols in `NameAndTypeEntryImpl` is quite complex, can we move it to a utility method in `Util` for cleaner call sites?
3. `parameterSlots` `parseParameterSlots` `maxLocals` in `Util` can probably operate on a `MethodTypeDesc` than a bitset, especially that the method type symbols are available in most scenarios already.
4. `StackMapGenerator.processInvokeInstructions` can probably scan through the `MethodTypeDesc` instead of using the hand-rolled `while (++pos < descLen && (ch = mDesc.charAt(pos)) != ')')` loop. In fact, the whole loop can be replaced by `Util.parameterSlots()`
5. `StackMapGenerator.isDoubleSlot(ClassDesc)` should be moved to `Util` and used by `parameterSlots()` etc (assuming they've migrated to `MethodTypeDesc`), and implementation be updated to:

    public static boolean isDoubleSlot(ClassDesc desc) {
        return desc.isPrimitive() && (desc.charAt(0) == 'D' || desc.charAt(0) == 'J');
    }

to avoid potentially slow string comparisons.

Thanks adam! Also if you can, I wish you can respond to mandy at https://github.com/openjdk/jdk/pull/13598#issuecomment-1524411693 on why we need an api to obtain the internal name (or a string for CONSTANT_Class_info) from a ClassDesc.

Though the proposal to add method to obtain internal names on ClassDesc is rejected, I think we can still obtain a performance boost if we can avoid frequent conversion between internal names and descriptors.

Looking at the usages of `Util.toClassDesc` and `Util.toInternalName`, we can already cache result of `toClassDesc` in `ClassEntry`. We can think similarly for `toInternalName`: the hash for `ClassEntry` may be derived from the class descriptor than the class internal name, so we don't need to call `toInternalName` every time we want to look up a ClassEntry with a ClassDesc in a constant pool.

If we can hash internal names for constant pool without calling Util.toInternalName on ClassDesc instances, can that offset the performance penalty of toInternalName?

I have a proof-of-concept patch here https://github.com/liachmodded/jdk/commit/3e6aa523521085e99b52fb49633676166910100f
which hashes class entries by the symbol's full descriptors instead of by the index of the linked utf8; we can access the hash of a full descriptor given the hash of an internal name easily, should be faster than iterating over or creating the internal name for a hash.

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

PR Comment: https://git.openjdk.org/jdk/pull/13671#issuecomment-1523668329
PR Comment: https://git.openjdk.org/jdk/pull/13671#issuecomment-1525083666
PR Comment: https://git.openjdk.org/jdk/pull/13671#issuecomment-1526287425
PR Comment: https://git.openjdk.org/jdk/pull/13671#issuecomment-1527477752
PR Comment: https://git.openjdk.org/jdk/pull/13671#issuecomment-1527621920