On Thu, 8 Jun 2023 16:37:22 GMT, Adam Sotona <asot...@openjdk.org> wrote:
>> Classfile context object and multi-state options have been discussed at >> https://mail.openjdk.org/pipermail/classfile-api-dev/2023-May/000321.html >> This patch implements the proposed changes in Classfile API and fixes all >> affected code across JDK sources and tests. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with two additional > commits since the last revision: > > - proposed semi-synchronized caching, where the map is not locked during > delegate call > - used Factory.INSTANCE for system ClassHierarchyResolver cache I've performed a lot of benchmarking and the key result is that introduction of `Classfile` context (or rather say context specific CHR instead of global static field) cause a lot of regressions in our benchmarks. Below are aggregated only relative results of benchmarks with significant regressions. Current `master` with global static cache instance is the base for relative regressions in the table. - `ConcurrentHashMap::computeIfAbsent` represents the initial fully-synchronized cache implementation. - `HashMap::computeIfAbsent` is the thread-unsafe option. - `ConcurrentHashMap::get and put` is the lazy-synchronized solution - **Always create a fresh context with an empty cache** is what Brian proposed as one of the options - **Class loading with local cache only** is a step back to what ASM is using. Interesting point is that **Always create a fresh context with an empty cache** works with similar performance as the others, except for `GenerateStackMaps.benchmark`, where it causes significant regression. **Class loading with local cache only** assumes `ClassLoader` would do the caching for us. Even the class loading is initially slower and does not work for all use cases, it may be the best for the default CHR. And we could skip any `CHR::cached` in the API. Also interesting is that `ConcurrentHashMap::computeIfAbsent` cause 10% regression of **ParseOptions.transformNoStackmap** benchmark, where it should have no or minimal effect. | `ConcurrentHashMap::computeIfAbsent` | `HashMap::computeIfAbsent` | `ConcurrentHashMap::get and put` | Always create a fresh context with an empty cache | Class loading with local cache only -- | -- | -- | -- | -- | -- AdHocAdapt.transform LIFT | -19% | -17% | -16% | -17% | -17% AdHocAdapt.transform LIFT1 | -24% | -25% | -23% | -26% | -23% AdHocAdapt.transform LIFT2 | -23% | -26% | -23% | -25% | -23% AdaptInjectNoop.transform NOP_SHARED | -16% | -19% | -16% | -18% | -7% AdaptNull.transform SHARED_3 | -18% | -21% | -19% | -17% | -18% AdaptNull.transform SHARED_3_NO_DEBUG | -22% | -22% | -25% | -21% | -22% GenerateStackMaps.benchmark | | | | -27% | ParseOptions.transformNoDebug | -31% | -20% | -20% | -19% | -18% ParseOptions.transformNoLineNumbers | -25% | -24% | -20% | -20% | -20% ParseOptions.transformNoStackmap | -10% | | | | ------------- PR Comment: https://git.openjdk.org/jdk/pull/14180#issuecomment-1584430448
