neoremind edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation URL: https://github.com/apache/calcite/pull/1875#issuecomment-611042597 @vlsi Thank you for your quick response. I have added new benchmark test case for baseline to disable guava caching. The result shows as below (code is in [my branch](https://github.com/neoremind/calcite/blob/refvisit_benchmark/ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/ReflectiveVisitDispatcherTest2.java)). ``` Benchmark (guavaCacheMaxSize) Mode Cnt Score Error Units ReflectiveVisitDispatcherTest102.testGlobalCachingUsingGuavaCache 0 avgt 5 116.446 ± 8.957 ns/op ReflectiveVisitDispatcherTest102.testGlobalCachingUsingGuavaCache 128 avgt 5 133.678 ± 8.251 ns/op ReflectiveVisitDispatcherTest102.testGlobalCachingUsingHashMap 0 avgt 5 73.213 ± 7.614 ns/op ReflectiveVisitDispatcherTest102.testInstanceCachingWithReflection 0 avgt 5 227.130 ± 9.033 ns/op ``` Table title are operations performed by each solution. | | lookup method | get by key from map | put key value to map | time cost | -|-|-|-|-| `testGlobalCachingUsingGuavaCache with guavaCacheMaxSize = 0` (Disable cache, looking up method every time for each call) | Y | | | 116.446 ns/op | `testGlobalCachingUsingGuavaCache with guavaCacheMaxSize = 128` (Global cache with guava cache) | | Y | | 133.678 ns/op | `testGlobalCachingUsingHashMap` (Global cache with JDK ConcurrentHashMap) | | Y | | 73.213 ns/op | `testInstanceCachingWithReflection` (Original implementation to cache per instance for each call) | Y | Y | Y | 227.130 ns/op | The conclusion we could get is 1. Guava cache overhead is a little too big, compared to JDK ConcurrentHashMap. But JDK ConcurrentHashMap is not limit sized, so in the previous discussion, we do not choose JDK ConcurrentHashMap. 2. Guava cache overhead is even bigger than looking up method for each call. This is what surprised me. 3. The original implementation is the slowest one because it has to get from map, look up method then put method to map, it combines all the operations, the solution works well in the same instance, but for every Calcite invocation it has to sacrifice performance to look up method every time, that is where the PR tries to improve. The current version does improve performance and balance memory usage. But it is slower than 1) JDK ConcurrentHashMap for caching (but not limit size), 2) looking up method direct in each call without map operations. PS. Correct me if I am wrong, there is no `softKeys` api in guava, actually I find places in Calcite to hold classes as key in HashMap as global caching, like `org.apache.calcite.sql2rel.ReflectiveConvertletTable`, so I was wondering if global caching with JDK ConcurrentHashMap could work here as well. Looking forward to your advice.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
