RFR: 8308899: Introduce Classfile.Context and improve Classfile.Option(s) [v11]

[Brian Goetz]

On Thu, 8 Jun 2023 16:37:22 GMT, Adam Sotona <asotona at 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

FWIW: I have found AdaptNull.SHARED_3 to be among the most useful 
benchmarks for finding unexpected regressions.

I think there's something fishy going on.  Given that the baseline 
(static shared cache) has to protect against concurrent access already, 
and that there should be little or no other significant difference 
between the static version and the context version aside from caching, I 
can't see how those differences explain these regressions.  Are we 
making an apples-to-apples comparison of what CHR algorithm we're using? 
Is the creation of the CHR instances is perturbing the results?

Let's try to take CHR operation out of the equation: runt both baseline 
and context versions using a CHR that does almost nothing (e.g., says 
LUB(a,b) is always Object) or which uses a precomputed static cache.  
I'm guessing this shows no regression, and if so, this will likely tell 
us that the _context_ refactor is not the problem, but attempting to 
unshare the CHR cache.  Then we can drill into that problem.



On 6/9/2023 7:31 AM, Adam Sotona wrote:
>
> I've performed a lot of benchmarking and the key result is that 
> introduction of |Classfile| context 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 a base for the 
> 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% 				
>
> —
> Reply to this email directly, view it on GitHub 
> <https://github.com/openjdk/jdk/pull/14180#issuecomment-1584430448>, 
> or unsubscribe 
> <https://github.com/notifications/unsubscribe-auth/AABJ4RD7CBCQU3MSP4LJVILXKMCP3ANCNFSM6AAAAAAYQLACYQ>.
> You are receiving this because you commented.Message ID: 
> ***@***.***>
>

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

PR Comment: https://git.openjdk.org/jdk/pull/14180#issuecomment-1584445113