You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/03/25 15:52:15 UTC
[GitHub] [calcite] neoremind opened a new pull request #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
neoremind opened a new pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875
By examining a simple query through flame graph (see [issue](https://issues.apache.org/jira/browse/CALCITE-3873)), one interesting point is that I find there are too many calls using reflection, which is not performant, although the total overhead is less than 1%, I still spend some time trying to improve. Most invocations are rooted down to `ReflectiveVisitDispatcher`, the current implementation creates new instance whenever needed, and looking up methods by reflection per instance, I think by caching methods globally, as the methods count is countable to 68 possible places, different `ReflectiveVisitDispatcher` in different thread is able to reuse. The fundamental change will benefit other likewise invocations as well.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-606988861
@vlsi Would you help me review the PR again? Many thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r401480724
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##########
@@ -332,6 +332,23 @@
intProperty("calcite.bindable.cache.concurrencyLevel", 1,
v -> v >= 1 && v <= Integer.MAX_VALUE);
+ /**
+ * The maximum size of the cache used for storing methods by
+ * {@link org.apache.calcite.util.ReflectiveVisitDispatcher}.
+ *
+ * <p>The default value is 128.</p>
+ *
+ * <p>The property can take any value between [1, 256] inclusive. If the
+ * value is not valid (or not specified) then the default value is used.</p>
+ *
+ * <p>Looking up method by reflection invocation is slow, caching technique will
+ * mitigate the overhead.</p>
+ */
+ public static final CalciteSystemProperty<Integer>
+ REFLECT_VISIT_DISPATCHER_METHOD_CACHE_MAX_SIZE =
+ intProperty("calcite.reflect.visit.dispatcher.method.cache.maxSize", 128,
+ v -> v > 0 && v <= 256);
Review comment:
The best practice is:
1) Have sensible defaults. I agree that making the cache with a limit of 128 by default is good.
2) Provide a way to deactivate the feature. There might be cases where the feature introduces regressions. E.g. memory leaks. Then, a flag to disable the feature will help until the bug is resolved. For instance, `maximum.size=0` might mean `cache is disabled`
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r398586118
##########
File path: core/src/main/java/org/apache/calcite/util/ReflectVisitDispatcherImpl.java
##########
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import org.apache.calcite.config.CalciteSystemProperty;
+
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableList;
+
+import java.lang.reflect.Method;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * Looking up methods relating to reflective visitation. Caching reflect
+ * method globally, every instance of the class shares the global cache to
+ * mitigating java reflection invocation overhead.
+ *
Review comment:
Use mitigate as a verb
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
hsyuan commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-611873148
`softValues` is being used by Calcite, and in our production system we never saw cache OOM again after patch: https://github.com/apache/calcite/commit/ccbacf624c60b8b707c9e219a58d3968a9d4b854#diff-1f7223a50fa7f290893cee6fec9318a2R57
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind closed pull request #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind closed pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi edited a comment on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
vlsi edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-610846372
> it describes "Weak values will be garbage collected once they are weakly reachable. This makes them a poor candidate for caching; consider {@link #softValues} instead."
The keys can still reference classes and retain them from the garbage collector.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r398234467
##########
File path: core/src/main/java/org/apache/calcite/util/ReflectUtil.java
##########
@@ -403,71 +403,40 @@ private static Method lookupVisitMethod(
}
/**
- * Creates a dispatcher for calls to {@link #lookupVisitMethod}. The
- * dispatcher caches methods between invocations.
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}. By default the
+ * dispatcher caches methods globally. If caching methods between invocations
+ * is preferred, use the overridden method {@link #createDispatcher(Class, Class, boolean)}.
*
- * @param visitorBaseClazz Visitor base class
- * @param visiteeBaseClazz Visitee base class
+ * @param visitorBaseClazz Visitor base class
+ * @param visiteeBaseClazz Visitee base class
* @return cache of methods
*/
public static <R extends ReflectiveVisitor, E> ReflectiveVisitDispatcher<R, E> createDispatcher(
final Class<R> visitorBaseClazz,
final Class<E> visiteeBaseClazz) {
+ return createDispatcher(visitorBaseClazz, visiteeBaseClazz, true);
+ }
+
+ /**
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}.
+ *
+ * @param visitorBaseClazz Visitor base class
+ * @param visiteeBaseClazz Visitee base class
+ * @param useGlobalMethodCache If set to true, the created dispatchers will
+ * share a globally cache to store methods to
+ * mitigating reflection invocation overhead
+ * when looking up method. If set to false, every
+ * dispatcher instance will only cache methods
+ * between invocations individually.
+ * @return cache of methods
+ */
+ public static <R extends ReflectiveVisitor, E> ReflectiveVisitDispatcher<R, E> createDispatcher(
+ final Class<R> visitorBaseClazz,
+ final Class<E> visiteeBaseClazz,
+ final boolean useGlobalMethodCache) {
Review comment:
Perhaps we don't need this configuration, always use cache. Any reason that will not use cache?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind edited a comment on issue #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-607168132
@vlsi I have addressed your comments.
1) Set max cache to 0 to disable global caching.
2) Use `CacheBuilder.newBuilder().softValues()`, does not use weak values, because I think soft references are more commonly used to implement memory-sensitive caches, they live longer until before an OOM is thrown.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
danny0405 commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-611905665
Thanks, keeps only the benchmark test makes sense to me.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi edited a comment on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
vlsi edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-607098114
It looks good, except for the fact that GLOBAL_METHOD_CACHE might result in a memory leak.
It would retain `Method` (and thus `Class`) objects preventing them from being garbage-collected.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind edited a comment on issue #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-611891530
@hsyuan Thanks for your advice about softValues. Actually now I am a little reluctant to the change, because according to benchmark, by using Guava global cache, the overhead is a little bigger than just looking up method by Reflection every time (<20 ns/op gap), and the most performant option is to use JDK ConcurrentHashMap as global cache but without size limitation and expiration for GC. The master branch version may call Reflection the first time, and then will use JDK HashMap as cache in instance level which does good.
So my opinion is, if we can not use JDK ConcurrentHashMap as global cache, then the update may no longer necessary. Hopefully the unit test and benchmark could be useful after all. Sorry for making the thread so long.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1875: [CALCITE-3873] Use global
caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
vlsi commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-610846372
> it describes "Weak values will be garbage collected once they are weakly reachable. This makes them a poor candidate for caching; consider {@link #softValues} instead."
The keys can still reference classes and retain them from garbage collecting.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r405375307
##########
File path: ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/ReflectiveVisitDispatcherTest.java
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.benchmarks;
+
+import org.apache.calcite.benchmarks.helper.People;
+import org.apache.calcite.util.ReflectUtil;
+import org.apache.calcite.util.ReflectiveVisitor;
+
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.profile.GCProfiler;
+import org.openjdk.jmh.runner.Runner;
+import org.openjdk.jmh.runner.RunnerException;
+import org.openjdk.jmh.runner.options.Options;
+import org.openjdk.jmh.runner.options.OptionsBuilder;
+
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Benchmark for {@link org.apache.calcite.util.ReflectiveVisitDispatcherImpl}
+ */
+@Fork(value = 1)
+@Measurement(iterations = 5, time = 10, timeUnit = TimeUnit.SECONDS)
+@Warmup(iterations = 1, time = 10, timeUnit = TimeUnit.SECONDS)
+@BenchmarkMode(Mode.AverageTime)
+@OutputTimeUnit(TimeUnit.NANOSECONDS)
+@State(Scope.Benchmark)
+public class ReflectiveVisitDispatcherTest {
+
+ private ReflectiveVisitor visitor = new People();
+
+ @Benchmark
+ public String testReflectiveVisitorDispatcherInvoke() {
+ ReflectUtil.MethodDispatcher<String> dispatcher = ReflectUtil.createMethodDispatcher(
+ String.class, visitor, "say", String.class);
+ String result = dispatcher.invoke("hello");
+ return result;
+ }
Review comment:
Please
1) Do not use literals in benchmarks. That is all the parameters should come from `@State` fields.
See https://shipilev.net/blog/2014/java-scala-divided-we-fail/
ReflectUtil
2) Please add `baseline()` method that would perform direct computation (without `ReflectUtil`) so the cost of the dispatcher is visible
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-607168132
@vlsi I have addressed your comments.
1) Set max cache to 0 to disable global caching.
2) Use `CacheBuilder.newBuilder().softValues()`, does not use weak values, because I think soft references are most commonly used to implement memory-sensitive caches, they lives longer until before an OOM is thrown.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind edited a comment on issue #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-607168132
@vlsi I have addressed your comments.
1) Set max cache to 0 to disable global caching.
2) Use `CacheBuilder.newBuilder().softValues()`, does not use weak values, because I think soft references are more commonly used to implement memory-sensitive caches, they live longer until before an OOM is thrown. In Guava javadoc, it describes "Weak values will be garbage collected once they are weakly reachable. This makes them a poor candidate for caching; consider {@link #softValues} instead."
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1875: [CALCITE-3873] Use global
caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
vlsi commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-607143698
>I see, how about using weak or soft values?
Weak would probably work.
Soft references are almost always a bad choice.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-604520763
@vlsi I will try to add the benchmark. `MethodHandle` would be a good alternative, since it requires both returned and parameter types, it might take some time to update the code to make it work. I will look into this solution.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind edited a comment on issue #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-605091418
@vlsi I have add benchmark to `ubenchmark` module.
As @hsyuan suggested, I make global caching by default. In order to benchmark against current instance level caching using `Reflection` and `MethodHandle`, especially for `MethodHandle`, classes like `ReflectUtil` and `ReflectiveVisitDispatcher` need to be updated much, so I make that change in my [fork branch](https://github.com/neoremind/calcite/blob/refvisit_benchmark/ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/ReflectiveVisitDispatcherTest.java).
The test result is a little out of expectation.
```
Benchmark Mode Cnt Score Error Units
ReflectiveVisitDispatcherTest.testReflectiveVisitorDispatcherInvokeGlobalCaching avgt 3 143.483 ± 32.596 ns/op
ReflectiveVisitDispatcherTest.testReflectiveVisitorDispatcherInvokeInstanceCaching avgt 3 290.380 ± 857.199 ns/op
ReflectiveVisitDispatcherTest.testReflectiveVisitorDispatcherInvokeInstanceCachingUseMethodHandle avgt 3 2487.406 ± 1083.180 ns/op
```
PS. I refine test to return generated result from the benchmark methods, update benchmark parameters like fork and iteration times.
MethodHandle is way slow than reflection, the below part is performance killer. If I make candidateMethod to be feteched from ThreadLocal which I initialized explicitly, the average time can drop down to about 208 ns/op.
```
MethodType mt = MethodType.methodType(returnType, paramTypes);
MethodHandle candidateMethod = MethodHandles.lookup().findVirtual(visitorClass, visitMethodName, mt);
```
I will look into the reason if I have more bandwidth.
My benchmark result is pretty much the same with [JavaReflectionButMuchFaster.html](http://www.jboss.org/optaplanner/blog/2018/01/09/JavaReflectionButMuchFaster.html).
```
DirectAccess avgt 60 2.590 ± 0.014 ns/op
Reflection avgt 60 5.275 ± 0.053 ns/op // 104% slower
MethodHandle avgt 60 6.100 ± 0.079 ns/op // 136% slower
```
We need the reflection agility to be able to find methods at runtime, `MethodHanlde` seems not a good choice.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-605091418
@vlsi , as @hsyuan suggested, I make global caching by default. In order to benchmark against current instance level caching using `Reflection` and `MethodHandle`, especially for `MethodHandle`, classes like `ReflectUtil` and `ReflectiveVisitDispatcher` need to be updated much, so I make that change in my [fork branch](https://github.com/neoremind/calcite/blob/refvisit_benchmark/ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/ReflectiveVisitDispatcherTest.java).
The test result is a little out of expectation.
```
Benchmark Mode Cnt Score Error Units
ReflectiveVisitDispatcherTest.testReflectiveVisitorDispatcherInvokeGlobalCaching avgt 3 143.483 ± 32.596 ns/op
ReflectiveVisitDispatcherTest.testReflectiveVisitorDispatcherInvokeInstanceCaching avgt 3 290.380 ± 857.199 ns/op
ReflectiveVisitDispatcherTest.testReflectiveVisitorDispatcherInvokeInstanceCachingUseMethodHandle avgt 3 2487.406 ± 1083.180 ns/op
```
PS. I refine test to return generated result from the benchmark methods, update benchmark parameters like fork and iteration times.
MethodHandle is way slow than reflection, the below part is performance killer. If I make candidateMethod to be feteched from ThreadLocal which I initialized explicitly, the average time can drop down to about 208 ns/op.
```
MethodType mt = MethodType.methodType(returnType, paramTypes);
MethodHandle candidateMethod = MethodHandles.lookup().findVirtual(visitorClass, visitMethodName, mt);
```
I will look into the reason if I have more bandwidth.
My benchmark result is pretty much the same with [JavaReflectionButMuchFaster.html](http://www.jboss.org/optaplanner/blog/2018/01/09/JavaReflectionButMuchFaster.html).
```
DirectAccess avgt 60 2.590 ± 0.014 ns/op
Reflection avgt 60 5.275 ± 0.053 ns/op // 104% slower
MethodHandle avgt 60 6.100 ± 0.079 ns/op // 136% slower
```
We need the reflection agility to be able to find methods at runtime, `MethodHanlde` seems not a good choice.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r401494623
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##########
@@ -332,6 +332,23 @@
intProperty("calcite.bindable.cache.concurrencyLevel", 1,
v -> v >= 1 && v <= Integer.MAX_VALUE);
+ /**
+ * The maximum size of the cache used for storing methods by
+ * {@link org.apache.calcite.util.ReflectiveVisitDispatcher}.
+ *
+ * <p>The default value is 128.</p>
+ *
+ * <p>The property can take any value between [1, 256] inclusive. If the
+ * value is not valid (or not specified) then the default value is used.</p>
+ *
+ * <p>Looking up method by reflection invocation is slow, caching technique will
+ * mitigate the overhead.</p>
+ */
+ public static final CalciteSystemProperty<Integer>
+ REFLECT_VISIT_DISPATCHER_METHOD_CACHE_MAX_SIZE =
+ intProperty("calcite.reflect.visit.dispatcher.method.cache.maxSize", 128,
+ v -> v > 0 && v <= 256);
Review comment:
got it, I will update later.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r401422961
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##########
@@ -332,6 +332,23 @@
intProperty("calcite.bindable.cache.concurrencyLevel", 1,
v -> v >= 1 && v <= Integer.MAX_VALUE);
+ /**
+ * The maximum size of the cache used for storing methods by
+ * {@link org.apache.calcite.util.ReflectiveVisitDispatcher}.
+ *
+ * <p>The default value is 128.</p>
+ *
+ * <p>The property can take any value between [1, 256] inclusive. If the
+ * value is not valid (or not specified) then the default value is used.</p>
+ *
+ * <p>Looking up method by reflection invocation is slow, caching technique will
+ * mitigate the overhead.</p>
+ */
+ public static final CalciteSystemProperty<Integer>
+ REFLECT_VISIT_DISPATCHER_METHOD_CACHE_MAX_SIZE =
+ intProperty("calcite.reflect.visit.dispatcher.method.cache.maxSize", 128,
+ v -> v > 0 && v <= 256);
Review comment:
Why do you limit the value with 256?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r401439918
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##########
@@ -332,6 +332,23 @@
intProperty("calcite.bindable.cache.concurrencyLevel", 1,
v -> v >= 1 && v <= Integer.MAX_VALUE);
+ /**
+ * The maximum size of the cache used for storing methods by
+ * {@link org.apache.calcite.util.ReflectiveVisitDispatcher}.
+ *
+ * <p>The default value is 128.</p>
+ *
+ * <p>The property can take any value between [1, 256] inclusive. If the
+ * value is not valid (or not specified) then the default value is used.</p>
+ *
+ * <p>Looking up method by reflection invocation is slow, caching technique will
+ * mitigate the overhead.</p>
+ */
+ public static final CalciteSystemProperty<Integer>
+ REFLECT_VISIT_DISPATCHER_METHOD_CACHE_MAX_SIZE =
+ intProperty("calcite.reflect.visit.dispatcher.method.cache.maxSize", 128,
+ v -> v > 0 && v <= 256);
Review comment:
I count the reference of the method: `ReflectUtil#createMethodDispatcher` and `ReflectUtil#createDispatcher`, there are 68 places might be called through ReflectUtil (see below), the methods we are caching is limited, so I initialize the max cache size to 128 to hold them on, upper limit to 256 (answer your below question) in case future updates might use this feature as well, so it won't be a big burden as they stay in memory. In my opinion, I would rather call it a global caching, not memory leak, because we might use them repeatedly.
```
org.apache.calcite.rel.rel2sql.RelToSqlConverter#visit: 18 overloading methods
org.apache.calcite.sql2rel.RelDecorrelator#decorrelateRel: 15 overloading methods
org.apache.calcite.sql2rel.RelFieldTrimmer#trimFields: 11 overloading methods
org.apache.calcite.sql2rel.RelStructuredTypeFlattener#rewriteRel: 22 overloading methods
org.apache.calcite.interpreter.Interpreter.CompilerImpl#rewrite: 1 method.
org.apache.calcite.interpreter.Interpreter.CompilerImpl#visit: 1 method.
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r398391089
##########
File path: core/src/main/java/org/apache/calcite/util/ReflectUtil.java
##########
@@ -403,71 +403,40 @@ private static Method lookupVisitMethod(
}
/**
- * Creates a dispatcher for calls to {@link #lookupVisitMethod}. The
- * dispatcher caches methods between invocations.
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}. By default the
+ * dispatcher caches methods globally. If caching methods between invocations
+ * is preferred, use the overridden method {@link #createDispatcher(Class, Class, boolean)}.
*
- * @param visitorBaseClazz Visitor base class
- * @param visiteeBaseClazz Visitee base class
+ * @param visitorBaseClazz Visitor base class
+ * @param visiteeBaseClazz Visitee base class
* @return cache of methods
*/
public static <R extends ReflectiveVisitor, E> ReflectiveVisitDispatcher<R, E> createDispatcher(
final Class<R> visitorBaseClazz,
final Class<E> visiteeBaseClazz) {
+ return createDispatcher(visitorBaseClazz, visiteeBaseClazz, true);
+ }
+
+ /**
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}.
+ *
+ * @param visitorBaseClazz Visitor base class
+ * @param visiteeBaseClazz Visitee base class
+ * @param useGlobalMethodCache If set to true, the created dispatchers will
+ * share a globally cache to store methods to
+ * mitigating reflection invocation overhead
+ * when looking up method. If set to false, every
+ * dispatcher instance will only cache methods
+ * between invocations individually.
+ * @return cache of methods
+ */
+ public static <R extends ReflectiveVisitor, E> ReflectiveVisitDispatcher<R, E> createDispatcher(
+ final Class<R> visitorBaseClazz,
+ final Class<E> visiteeBaseClazz,
+ final boolean useGlobalMethodCache) {
Review comment:
Comment addressed. Make `ReflectiveVisitDispatcher` to always use global caching.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-604957529
nice, it works 👏
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r401439918
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##########
@@ -332,6 +332,23 @@
intProperty("calcite.bindable.cache.concurrencyLevel", 1,
v -> v >= 1 && v <= Integer.MAX_VALUE);
+ /**
+ * The maximum size of the cache used for storing methods by
+ * {@link org.apache.calcite.util.ReflectiveVisitDispatcher}.
+ *
+ * <p>The default value is 128.</p>
+ *
+ * <p>The property can take any value between [1, 256] inclusive. If the
+ * value is not valid (or not specified) then the default value is used.</p>
+ *
+ * <p>Looking up method by reflection invocation is slow, caching technique will
+ * mitigate the overhead.</p>
+ */
+ public static final CalciteSystemProperty<Integer>
+ REFLECT_VISIT_DISPATCHER_METHOD_CACHE_MAX_SIZE =
+ intProperty("calcite.reflect.visit.dispatcher.method.cache.maxSize", 128,
+ v -> v > 0 && v <= 256);
Review comment:
I count the reference of the method: `ReflectUtil#createMethodDispatcher` and `ReflectUtil#createDispatcher`, there are 68 places might be called through ReflectUtil (see below), the methods we are caching is limited, so I initialize the max cache size to 128 to hold them on, upper limit to 256 (answer your below question) to not make it not a big burden as they stay in memory. In my opinion, I would rather call it a global caching, not memory leak, because we might use them repeatedly.
```
org.apache.calcite.rel.rel2sql.RelToSqlConverter#visit: 18 overloading methods
org.apache.calcite.sql2rel.RelDecorrelator#decorrelateRel: 15 overloading methods
org.apache.calcite.sql2rel.RelFieldTrimmer#trimFields: 11 overloading methods
org.apache.calcite.sql2rel.RelStructuredTypeFlattener#rewriteRel: 22 overloading methods
org.apache.calcite.interpreter.Interpreter.CompilerImpl#rewrite: 1 method.
org.apache.calcite.interpreter.Interpreter.CompilerImpl#visit: 1 method.
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r398291034
##########
File path: core/src/main/java/org/apache/calcite/util/ReflectUtil.java
##########
@@ -403,71 +403,40 @@ private static Method lookupVisitMethod(
}
/**
- * Creates a dispatcher for calls to {@link #lookupVisitMethod}. The
- * dispatcher caches methods between invocations.
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}. By default the
+ * dispatcher caches methods globally. If caching methods between invocations
+ * is preferred, use the overridden method {@link #createDispatcher(Class, Class, boolean)}.
*
- * @param visitorBaseClazz Visitor base class
- * @param visiteeBaseClazz Visitee base class
+ * @param visitorBaseClazz Visitor base class
+ * @param visiteeBaseClazz Visitee base class
* @return cache of methods
*/
public static <R extends ReflectiveVisitor, E> ReflectiveVisitDispatcher<R, E> createDispatcher(
final Class<R> visitorBaseClazz,
final Class<E> visiteeBaseClazz) {
+ return createDispatcher(visitorBaseClazz, visiteeBaseClazz, true);
+ }
+
+ /**
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}.
+ *
+ * @param visitorBaseClazz Visitor base class
+ * @param visiteeBaseClazz Visitee base class
+ * @param useGlobalMethodCache If set to true, the created dispatchers will
+ * share a globally cache to store methods to
+ * mitigating reflection invocation overhead
+ * when looking up method. If set to false, every
+ * dispatcher instance will only cache methods
+ * between invocations individually.
+ * @return cache of methods
+ */
+ public static <R extends ReflectiveVisitor, E> ReflectiveVisitDispatcher<R, E> createDispatcher(
+ final Class<R> visitorBaseClazz,
+ final Class<E> visiteeBaseClazz,
+ final boolean useGlobalMethodCache) {
Review comment:
To maintain backward compatibility, I add one param boolean `useGlobalMethodCache` to method `ReflectiveVisitDispatcher<R, E> createDispatcher(..)` and provide an overridden method to make `useGlobalMethodCache=true` by default, so anywhere else won't be changed, but the underlying behavior will enable caching. In case there might be situations where `ReflectiveVisitDispatcher` is likely to cache per instance and does not want to bother the global cache, so to make that flexibility possible I made such change.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind edited a comment on issue #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-605091418
@vlsi I have added benchmark to `ubenchmark` module.
As @hsyuan suggested, I make global caching by default. In order to benchmark against current instance level caching using `Reflection` and `MethodHandle`, especially for `MethodHandle`, classes like `ReflectUtil` and `ReflectiveVisitDispatcher` need to be updated much, so I make that change in my [fork branch](https://github.com/neoremind/calcite/blob/refvisit_benchmark/ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/ReflectiveVisitDispatcherTest.java).
The test result is a little out of expectation.
```
Benchmark Mode Cnt Score Error Units
ReflectiveVisitDispatcherTest.testReflectiveVisitorDispatcherInvokeGlobalCaching avgt 3 143.483 ± 32.596 ns/op
ReflectiveVisitDispatcherTest.testReflectiveVisitorDispatcherInvokeInstanceCaching avgt 3 290.380 ± 857.199 ns/op
ReflectiveVisitDispatcherTest.testReflectiveVisitorDispatcherInvokeInstanceCachingUseMethodHandle avgt 3 2487.406 ± 1083.180 ns/op
```
PS. I refine test to return generated result from the benchmark methods, update benchmark parameters like fork and iteration times.
MethodHandle is way slow than reflection, the below part is performance killer. If I make candidateMethod to be feteched from ThreadLocal which I initialized explicitly, the average time can drop down to about 208 ns/op.
```
MethodType mt = MethodType.methodType(returnType, paramTypes);
MethodHandle candidateMethod = MethodHandles.lookup().findVirtual(visitorClass, visitMethodName, mt);
```
I will look into the reason if I have more bandwidth.
My benchmark result is pretty much the same with [JavaReflectionButMuchFaster.html](http://www.jboss.org/optaplanner/blog/2018/01/09/JavaReflectionButMuchFaster.html).
```
DirectAccess avgt 60 2.590 ± 0.014 ns/op
Reflection avgt 60 5.275 ± 0.053 ns/op // 104% slower
MethodHandle avgt 60 6.100 ± 0.079 ns/op // 136% slower
```
We need the reflection agility to be able to find methods at runtime, `MethodHanlde` seems not a good choice.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind edited a comment on issue #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-611911050
@danny0405 Many thanks for your advice. Can I close this PR and open a new one only to add unit test and benchmark? Also @vlsi do you think it ok?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind edited a comment on issue #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-605405488
@vlsi I refined the benchmark in [my branch](https://github.com/neoremind/calcite/blob/refvisit_benchmark/ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/ReflectiveVisitDispatcherTest.java).
The result is as below.
```
Benchmark Mode Cnt Score Error Units
ReflectiveVisitDispatcherTest.testGlobalCaching avgt 3 137.270 ± 32.275 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithLambdaFactory avgt 3 92459.660 ± 56777.698 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithLambdaFactoryThreadLocalInitialize avgt 3 199.687 ± 24.755 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithMethodHandle avgt 3 2421.131 ± 633.756 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithMethodHandleThreadLocalInitialize avgt 3 218.616 ± 41.754 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithReflection avgt 3 224.603 ± 22.770 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithReflectionThreadLocalInitialize avgt 3 218.406 ± 9.620 ns/op
```
From the result, it seems the pre-work is time consuming for both `MethodHandle` and `LambdaFactory` (built upon MethodHandle). To eliminate the initialization effect, I try to use ThreadLocal to initialize once, the result do verifies that, in terms of time cost, LambdaFactory < Reflection = MethodHandle, but the gap is very small.
Overall, back to what the PR brings, the global caching stragety with reflection invocation can satisfy our requirement during the whole process, instead of caching per instance or per thread.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1875: [CALCITE-3873] Use global
caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
vlsi commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-604937327
@neoremind , https://github.com/apache/calcite/pull/1880 makes `.main` classes runnable for me.
Please try it in your own branch (or even include 1880 changes to this PR).
I don't commit it yet because there's a pending discussion (hey, @danny0405 👋, no rush, but, your input is very welcome 😄 ) re https://github.com/apache/calcite/pull/1872 (see https://issues.apache.org/jira/browse/CALCITE-3871).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented 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 results 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-610838697
@vlsi sorry to bother you, would you take a look of my update again? I'd like to move forward this PR, many thanks 😄
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-604479873
@danny0405 jmh test result is as below. It shows by using reflection invocation to look up methods in every call, the overhead is not small compared with global caching. In real case, reflection invocation can happen between invocations and different `ReflectVisitorDispatcher` instance. The solution might be trivial, but I think it goes toward the right way.
```
Benchmark Mode Cnt Score Error Units
ReflectVisitorDispatcherTest.testGlobalLevelCaching avgt 15 108.398 ± 15.280 ns/op
ReflectVisitorDispatcherTest.testInstanceLevelCaching avgt 15 730.208 ± 117.693 ns/op
```
Test source code is attached in [JIRA](https://issues.apache.org/jira/secure/attachment/12997903/ReflectVisitorDispatcherTest.java), the full test report can be found [here](https://issues.apache.org/jira/secure/attachment/12997904/jmh_result.txt)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
hsyuan commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-611872164
> > it describes "Weak values will be garbage collected once they are weakly reachable. This makes them a poor candidate for caching; consider {@link #softValues} instead."
>
> The keys can still reference classes and retain them from the garbage collector.
@vlsi By using `softValues`, once the value is garbage collected, the reference to key will also be released.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r406600680
##########
File path: core/src/main/java/org/apache/calcite/util/ReflectiveVisitDispatcherImpl.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import org.apache.calcite.config.CalciteSystemProperty;
+
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableList;
+
+import java.lang.reflect.Method;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * Looking up methods relating to reflective visitation. Caching reflect
+ * method globally, every instance of the class shares the global cache to
+ * mitigate java reflection invocation overhead.
+ *
+ * @param <E> Argument type
+ * @param <R> Return type
+ */
+public class ReflectiveVisitDispatcherImpl<R extends ReflectiveVisitor, E>
+ implements ReflectiveVisitDispatcher<R, E> {
+
+ /**
+ * Global singleton cache with limited size used to cache methods. Every instance
+ * of the class will share this cache to mitigate java reflection invocation
+ * overhead when looking up methods.
+ * <p>Note that there should be multiple ways to implement caching, for example,
+ * <p>1) Passing <em>Cache</em> between objects and hopefully using Java's
+ * <em>RAII (Resource Acquisition Is Initialization)</em> like try-finally to
+ * garbage collect.
+ * <p>2) Using <code>ThreadLocal</code> to store cache per thread.
+ * <p>3) Using global cache.
+ * <p>Solution 1) and 2) could introduce complexity to the main course and it only
+ * works per instance or per thread.
+ * Solution 3) which introduces a controllable global small sized cache is a
+ * better solution to balance memory usage and performance.
+ */
+ private static final Cache<List<Object>, Method> GLOBAL_METHOD_CACHE =
Review comment:
Can we use `LoadingCache`?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r401474401
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##########
@@ -332,6 +332,23 @@
intProperty("calcite.bindable.cache.concurrencyLevel", 1,
v -> v >= 1 && v <= Integer.MAX_VALUE);
+ /**
+ * The maximum size of the cache used for storing methods by
+ * {@link org.apache.calcite.util.ReflectiveVisitDispatcher}.
+ *
+ * <p>The default value is 128.</p>
+ *
+ * <p>The property can take any value between [1, 256] inclusive. If the
+ * value is not valid (or not specified) then the default value is used.</p>
+ *
+ * <p>Looking up method by reflection invocation is slow, caching technique will
+ * mitigate the overhead.</p>
+ */
+ public static final CalciteSystemProperty<Integer>
+ REFLECT_VISIT_DISPATCHER_METHOD_CACHE_MAX_SIZE =
+ intProperty("calcite.reflect.visit.dispatcher.method.cache.maxSize", 128,
+ v -> v > 0 && v <= 256);
Review comment:
I make it as configurable in the previous commit through constructor, so that the caller would make choice, but I take suggestion from hsyuan to make it always as global cache 😄
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1875: [CALCITE-3873] Use global
caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
vlsi commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-607098114
It looks good, except the fact that GLOBAL_METHOD_CACHE might result in a memory leak.
It would retain `Method` (and thus `Class`) objects preventing them from being garbage-collected.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind edited a comment on issue #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
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`, and I just notice that a new PR #1901 also uses global hashset and COW list in `JaninoRelMetadataProvider.java`, 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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] hsyuan commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r398303036
##########
File path: core/src/main/java/org/apache/calcite/util/ReflectUtil.java
##########
@@ -403,71 +403,40 @@ private static Method lookupVisitMethod(
}
/**
- * Creates a dispatcher for calls to {@link #lookupVisitMethod}. The
- * dispatcher caches methods between invocations.
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}. By default the
+ * dispatcher caches methods globally. If caching methods between invocations
+ * is preferred, use the overridden method {@link #createDispatcher(Class, Class, boolean)}.
*
- * @param visitorBaseClazz Visitor base class
- * @param visiteeBaseClazz Visitee base class
+ * @param visitorBaseClazz Visitor base class
+ * @param visiteeBaseClazz Visitee base class
* @return cache of methods
*/
public static <R extends ReflectiveVisitor, E> ReflectiveVisitDispatcher<R, E> createDispatcher(
final Class<R> visitorBaseClazz,
final Class<E> visiteeBaseClazz) {
+ return createDispatcher(visitorBaseClazz, visiteeBaseClazz, true);
+ }
+
+ /**
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}.
+ *
+ * @param visitorBaseClazz Visitor base class
+ * @param visiteeBaseClazz Visitee base class
+ * @param useGlobalMethodCache If set to true, the created dispatchers will
+ * share a globally cache to store methods to
+ * mitigating reflection invocation overhead
+ * when looking up method. If set to false, every
+ * dispatcher instance will only cache methods
+ * between invocations individually.
+ * @return cache of methods
+ */
+ public static <R extends ReflectiveVisitor, E> ReflectiveVisitDispatcher<R, E> createDispatcher(
+ final Class<R> visitorBaseClazz,
+ final Class<E> visiteeBaseClazz,
+ final boolean useGlobalMethodCache) {
Review comment:
There won't be seeable behavior change with the flag. We can always use global cache and remove the flag.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] zabetak commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
zabetak commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-615331318
Interesting discussion and analysis. @neoremind It may be worth adding (copy-paste) the conclusions in the JIRA case.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1875: [CALCITE-3873] Use global
caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
vlsi commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-605098579
Have you seen https://dzone.com/articles/hacking-lambda-expressions-in-java ?
> The invocation of functional interfaces with dynamically generated implementations using LambdaMetafactory is significantly faster than invocation through the Java Reflection API
It does not look like your branch is using `LambdaMetafactory` :)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r406614630
##########
File path: core/src/main/java/org/apache/calcite/util/ReflectiveVisitDispatcherImpl.java
##########
@@ -0,0 +1,134 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import org.apache.calcite.config.CalciteSystemProperty;
+
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableList;
+
+import java.lang.reflect.Method;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * Looking up methods relating to reflective visitation. Caching reflect
+ * method globally, every instance of the class shares the global cache to
+ * mitigate java reflection invocation overhead.
+ *
+ * @param <E> Argument type
+ * @param <R> Return type
+ */
+public class ReflectiveVisitDispatcherImpl<R extends ReflectiveVisitor, E>
+ implements ReflectiveVisitDispatcher<R, E> {
+
+ /**
+ * Global singleton cache with limited size used to cache methods. Every instance
+ * of the class will share this cache to mitigate java reflection invocation
+ * overhead when looking up methods.
+ * <p>Note that there should be multiple ways to implement caching, for example,
+ * <p>1) Passing <em>Cache</em> between objects and hopefully using Java's
+ * <em>RAII (Resource Acquisition Is Initialization)</em> like try-finally to
+ * garbage collect.
+ * <p>2) Using <code>ThreadLocal</code> to store cache per thread.
+ * <p>3) Using global cache.
+ * <p>Solution 1) and 2) could introduce complexity to the main course and it only
+ * works per instance or per thread.
+ * Solution 3) which introduces a controllable global small sized cache is a
+ * better solution to balance memory usage and performance.
+ */
+ private static final Cache<List<Object>, Method> GLOBAL_METHOD_CACHE =
Review comment:
Yes, good catch, method referencing of CacheLoader.from is more readable and less code is needed.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r398291034
##########
File path: core/src/main/java/org/apache/calcite/util/ReflectUtil.java
##########
@@ -403,71 +403,40 @@ private static Method lookupVisitMethod(
}
/**
- * Creates a dispatcher for calls to {@link #lookupVisitMethod}. The
- * dispatcher caches methods between invocations.
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}. By default the
+ * dispatcher caches methods globally. If caching methods between invocations
+ * is preferred, use the overridden method {@link #createDispatcher(Class, Class, boolean)}.
*
- * @param visitorBaseClazz Visitor base class
- * @param visiteeBaseClazz Visitee base class
+ * @param visitorBaseClazz Visitor base class
+ * @param visiteeBaseClazz Visitee base class
* @return cache of methods
*/
public static <R extends ReflectiveVisitor, E> ReflectiveVisitDispatcher<R, E> createDispatcher(
final Class<R> visitorBaseClazz,
final Class<E> visiteeBaseClazz) {
+ return createDispatcher(visitorBaseClazz, visiteeBaseClazz, true);
+ }
+
+ /**
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}.
+ *
+ * @param visitorBaseClazz Visitor base class
+ * @param visiteeBaseClazz Visitee base class
+ * @param useGlobalMethodCache If set to true, the created dispatchers will
+ * share a globally cache to store methods to
+ * mitigating reflection invocation overhead
+ * when looking up method. If set to false, every
+ * dispatcher instance will only cache methods
+ * between invocations individually.
+ * @return cache of methods
+ */
+ public static <R extends ReflectiveVisitor, E> ReflectiveVisitDispatcher<R, E> createDispatcher(
+ final Class<R> visitorBaseClazz,
+ final Class<E> visiteeBaseClazz,
+ final boolean useGlobalMethodCache) {
Review comment:
To maintain backward compatibility, I add one param boolean `useGlobalMethodCache` to method `ReflectiveVisitDispatcher<R, E> createDispatcher(..)` and provide an overridden method to make `useGlobalMethodCache=true` by default, so anywhere else won't be changed, but the underlying behavior will enable global caching. In case there might be situations where `ReflectiveVisitDispatcher` is likely to cache per instance and does not want to bother the global cache, so to make that flexibility possible I made such change.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-605105348
@vlsi good idea, let me try later.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind edited a comment on issue #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-607168132
@vlsi I have addressed your comments.
1) Set max cache to 0 to disable global caching.
2) Use `CacheBuilder.newBuilder().softValues()`, does not use weak values, because I think soft references are most commonly used to implement memory-sensitive caches, they live longer until before an OOM is thrown.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-607112581
@vlsi I replied my your comment in the code review section above.
PS. There are global caches existing like below, all of them reachable, except that the`Integer.MAX_VALUE` limit might exhaust memory.
```
public static final CalciteSystemProperty<Integer> METADATA_HANDLER_CACHE_MAXIMUM_SIZE =
intProperty("calcite.metadata.handler.cache.maximum.size", 1000);
public static final CalciteSystemProperty<Integer> BINDABLE_CACHE_MAX_SIZE =
intProperty("calcite.bindable.cache.maxSize", 0, v -> v >= 0 && v <= Integer.MAX_VALUE);
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-607135659
I see, how about using weak or soft values?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind edited a comment on issue #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-611891530
@hsyuan Thanks for your advice about softValues, [javadoc](https://guava.dev/releases/19.0/api/docs/com/google/common/cache/CacheBuilder.html) justifies : _Entries are automatically evicted from the cache when any of maximumSize, maximumWeight, expireAfterWrite, expireAfterAccess, weakKeys, weakValues, or softValues are requested._
Actually now I am a little reluctant to the change, because according to benchmark, by using Guava global cache, the overhead is a little bigger than just looking up method by Reflection every time (<20 ns/op gap), and the most performant option is to use JDK ConcurrentHashMap as global cache but without size limitation and expiration for GC. The master branch version may call Reflection the first time, and then will use JDK HashMap as cache in instance level which does not bad.
So my opinion is, if we can not use JDK ConcurrentHashMap as global cache, then the update may no longer necessary. Hopefully the unit test and benchmark could be useful after all. Sorry for making the thread so long.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] danny0405 commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r398304160
##########
File path: core/build.gradle.kts
##########
@@ -41,7 +41,6 @@ dependencies {
api("com.fasterxml.jackson.core:jackson-annotations")
api("org.apache.calcite.avatica:avatica-core")
- api("org.apiguardian:apiguardian-api")
Review comment:
There are so many un-expected change.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1875: [CALCITE-3873] Use global
caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
vlsi commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-607120781
>I would rather call it a global caching, not memory leak, because we might use them repeatedly.
What I mean is GLOBAL_CACHE keeps a strong reference to the class objects, thus effectively prevents class unloading even in the case the class is not used anymore.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-611911050
@danny0405 Many thanks for your advice. Can I close this PR and open a new one only to add unit test and benchmark?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r401456055
##########
File path: core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java
##########
@@ -332,6 +332,23 @@
intProperty("calcite.bindable.cache.concurrencyLevel", 1,
v -> v >= 1 && v <= Integer.MAX_VALUE);
+ /**
+ * The maximum size of the cache used for storing methods by
+ * {@link org.apache.calcite.util.ReflectiveVisitDispatcher}.
+ *
+ * <p>The default value is 128.</p>
+ *
+ * <p>The property can take any value between [1, 256] inclusive. If the
+ * value is not valid (or not specified) then the default value is used.</p>
+ *
+ * <p>Looking up method by reflection invocation is slow, caching technique will
+ * mitigate the overhead.</p>
+ */
+ public static final CalciteSystemProperty<Integer>
+ REFLECT_VISIT_DISPATCHER_METHOD_CACHE_MAX_SIZE =
+ intProperty("calcite.reflect.visit.dispatcher.method.cache.maxSize", 128,
+ v -> v > 0 && v <= 256);
Review comment:
Is there a way to disable the cache?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1875: [CALCITE-3873] Use global
caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
vlsi commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-604506267
@neoremind , do you think you could add the jmh test to Calcite as well (see https://github.com/apache/calcite/tree/master/ubenchmark )?
PS Have you considered MethodHandle-based approach?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on a change in pull request #1875:
[CALCITE-3873] Use global caching for ReflectiveVisitDispatcher
implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#discussion_r398636010
##########
File path: core/src/main/java/org/apache/calcite/util/ReflectVisitDispatcherImpl.java
##########
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.util;
+
+import org.apache.calcite.config.CalciteSystemProperty;
+
+import com.google.common.cache.Cache;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.collect.ImmutableList;
+
+import java.lang.reflect.Method;
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * Looking up methods relating to reflective visitation. Caching reflect
+ * method globally, every instance of the class shares the global cache to
+ * mitigating java reflection invocation overhead.
+ *
Review comment:
Thanks for pointing out typo :-)
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-615893085
I have copied conclusions to JIRA case. Many thanks to Vladimir, Haisheng, Danny, Chunwei and Stamatis to take time reviewing my code kindly 😃
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind edited a comment on issue #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-605405488
@vlsi I refined the benchmark in [my branch](https://github.com/neoremind/calcite/blob/refvisit_benchmark/ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/ReflectiveVisitDispatcherTest.java).
The result is as below.
```
Benchmark Mode Cnt Score Error Units
ReflectiveVisitDispatcherTest.testGlobalCaching avgt 3 137.270 ± 32.275 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithLambdaFactory avgt 3 92459.660 ± 56777.698 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithLambdaFactoryThreadLocalInitialize avgt 3 199.687 ± 24.755 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithMethodHandle avgt 3 2421.131 ± 633.756 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithMethodHandleThreadLocalInitialize avgt 3 218.616 ± 41.754 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithReflection avgt 3 224.603 ± 22.770 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithReflectionThreadLocalInitialize avgt 3 218.406 ± 9.620 ns/op
```
From the result, it seems the pre-work is time consuming for both `MethodHandle` and `LambdaFactory` (built upon MethodHandle). To eliminate the initialization effect, I try to use ThreadLocal to initialize once, the result do verifies that, in terms of time cost, LambdaFactory < Reflection <= MethodHandle, but the gap is very small.
Overall, back to what the PR brings, the global caching stragety with reflection invocation can satisfy our requirement during the whole process, instead of caching per instance or per thread.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-605405488
@vlsi I refined the benchmark in [my branch](https://github.com/neoremind/calcite/blob/refvisit_benchmark/ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/ReflectiveVisitDispatcherTest.java).
The result is as below.
```
Benchmark Mode Cnt Score Error Units
ReflectiveVisitDispatcherTest.testGlobalCaching avgt 3 137.270 ± 32.275 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithLambdaFactory avgt 3 92459.660 ± 56777.698 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithLambdaFactoryThreadLocalInitialize avgt 3 199.687 ± 24.755 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithMethodHandle avgt 3 2421.131 ± 633.756 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithMethodHandleThreadLocalInitialize avgt 3 218.616 ± 41.754 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithReflection avgt 3 224.603 ± 22.770 ns/op
ReflectiveVisitDispatcherTest.testInstanceCachingWithReflectionThreadLocalInitialize avgt 3 218.406 ± 9.620 ns/op
```
From the result, it seems the pre-work is time consuming for both `MethodHandle` and `LambdaFactory` (built upon MethodHandle). To eliminate the initialization effect, I try to use ThreadLocal to initialize once, the result do verifies that, in terms of time cost, LambdaFactory < MethodHandle = reflection, but the gap is very small.
Overall, back to what the PR brings, the global caching stragety with reflection invocation can satisfy our requirement during the whole process, instead of caching per instance or per thread.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind edited a comment on issue #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-605091418
@vlsi I have added benchmark to `ubenchmark` module.
As @hsyuan suggested, I make global caching by default. In order to benchmark against current instance level caching using `Reflection` and `MethodHandle`, especially for `MethodHandle`, classes like `ReflectUtil` and `ReflectiveVisitDispatcher` need to be updated much, so I make that change in my [fork branch](https://github.com/neoremind/calcite/blob/refvisit_benchmark/ubenchmark/src/jmh/java/org/apache/calcite/benchmarks/ReflectiveVisitDispatcherTest.java).
The test result is a little out of expectation.
```
Benchmark Mode Cnt Score Error Units
ReflectiveVisitDispatcherTest.testReflectiveVisitorDispatcherInvokeGlobalCaching avgt 3 143.483 ± 32.596 ns/op
ReflectiveVisitDispatcherTest.testReflectiveVisitorDispatcherInvokeInstanceCaching avgt 3 290.380 ± 857.199 ns/op
ReflectiveVisitDispatcherTest.testReflectiveVisitorDispatcherInvokeInstanceCachingUseMethodHandle avgt 3 2487.406 ± 1083.180 ns/op
```
PS. I refine test to return generated result from the benchmark methods, update benchmark parameters like fork and iteration times.
MethodHandle is way slow than reflection, the below part is performance killer. If I make candidateMethod to be feteched from ThreadLocal which I initialized explicitly, the average time can drop down to about 208 ns/op.
```
MethodType mt = MethodType.methodType(returnType, paramTypes);
MethodHandle candidateMethod = MethodHandles.lookup().findVirtual(visitorClass, visitMethodName, mt);
```
I will look into the reason if I have more bandwidth.
My benchmark result is pretty much the same with [JavaReflectionButMuchFaster.html](http://www.jboss.org/optaplanner/blog/2018/01/09/JavaReflectionButMuchFaster.html).
```
Benchmark Mode Cnt Score Error Units
===================================================
DirectAccess avgt 60 2.590 ± 0.014 ns/op
Reflection avgt 60 5.275 ± 0.053 ns/op // 104% slower
MethodHandle avgt 60 6.100 ± 0.079 ns/op // 136% slower
StaticMethodHandle avgt 60 2.635 ± 0.027 ns/op // 2% slower
JavaCompiler avgt 60 2.726 ± 0.026 ns/op // 5% slower
LambdaMetafactory avgt 60 3.453 ± 0.034 ns/op // 33% slower
```
We need the reflection agility to be able to find methods at runtime, `MethodHanlde` seems not a good choice.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind commented on issue #1875: [CALCITE-3873] Use
global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind commented on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-604889135
@vlsi Have you encountered the following exception when running test in `ubenchmark` module?
```
Exception in thread "main" java.lang.RuntimeException: ERROR: Unable to find the resource: /META-INF/BenchmarkList
at org.openjdk.jmh.runner.AbstractResourceReader.getReaders(AbstractResourceReader.java:98)
at org.openjdk.jmh.runner.BenchmarkList.find(BenchmarkList.java:122)
at org.openjdk.jmh.runner.Runner.internalRun(Runner.java:263)
at org.openjdk.jmh.runner.Runner.run(Runner.java:209)
at org.apache.calcite.benchmarks.PreconditionTest.main(PreconditionTest.java:51)
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] neoremind edited a comment on issue #1875: [CALCITE-3873]
Use global caching for ReflectiveVisitDispatcher implementation
Posted by GitBox <gi...@apache.org>.
neoremind edited a comment on issue #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
URL: https://github.com/apache/calcite/pull/1875#issuecomment-611891530
@hsyuan Thanks for your advice about softValues. Actually now I am a little reluctant to the change, because according to benchmark, by using Guava global cache, the overhead is a little bigger than just looking up method by Reflection every time (<20 ns/op gap), and the most performant option is to use JDK ConcurrentHashMap as global cache but without size limitation and expiration for GC. The master branch version may call Reflection the first time, and then will use JDK HashMap as cache in instance level which does not bad.
So my opinion is, if we can not use JDK ConcurrentHashMap as global cache, then the update may no longer necessary. Hopefully the unit test and benchmark could be useful after all. Sorry for making the thread so long.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services