On Wed, 26 Apr 2023 15:04:50 GMT, Adam Sotona <[email protected]> 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
