[GitHub] [calcite] neoremind commented on a change in pull request #1875: [CALCITE-3873] Use global caching for ReflectiveVisitDispatcher implementation
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 Argument type
+ * @param Return type
+ */
+public class ReflectiveVisitDispatcherImpl
+implements ReflectiveVisitDispatcher {
+
+ /**
+ * 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.
+ * Note that there should be multiple ways to implement caching, for
example,
+ * 1) Passing Cache between objects and hopefully using Java's
+ * RAII (Resource Acquisition Is Initialization) like try-finally to
+ * garbage collect.
+ * 2) Using ThreadLocal to store cache per thread.
+ * 3) Using global cache.
+ * 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, 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:
us...@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
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}.
+ *
+ * The default value is 128.
+ *
+ * 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.
+ *
+ * Looking up method by reflection invocation is slow, caching technique
will
+ * mitigate the overhead.
+ */
+ public static final CalciteSystemProperty
+ 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:
us...@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
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}.
+ *
+ * The default value is 128.
+ *
+ * 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.
+ *
+ * Looking up method by reflection invocation is slow, caching technique
will
+ * mitigate the overhead.
+ */
+ public static final CalciteSystemProperty
+ 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:
us...@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
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}.
+ *
+ * The default value is 128.
+ *
+ * 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.
+ *
+ * Looking up method by reflection invocation is slow, caching technique
will
+ * mitigate the overhead.
+ */
+ public static final CalciteSystemProperty
+ 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:
us...@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
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}.
+ *
+ * The default value is 128.
+ *
+ * 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.
+ *
+ * Looking up method by reflection invocation is slow, caching technique
will
+ * mitigate the overhead.
+ */
+ public static final CalciteSystemProperty
+ 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:
us...@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
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: us...@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
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 visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
* @return cache of methods
*/
public static ReflectiveVisitDispatcher createDispatcher(
final Class visitorBaseClazz,
final Class visiteeBaseClazz) {
+return createDispatcher(visitorBaseClazz, visiteeBaseClazz, true);
+ }
+
+ /**
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}.
+ *
+ * @param visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
+ * @param useGlobalMethodCacheIf 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 ReflectiveVisitDispatcher createDispatcher(
+ final Class visitorBaseClazz,
+ final Class 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:
us...@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
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 visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
* @return cache of methods
*/
public static ReflectiveVisitDispatcher createDispatcher(
final Class visitorBaseClazz,
final Class visiteeBaseClazz) {
+return createDispatcher(visitorBaseClazz, visiteeBaseClazz, true);
+ }
+
+ /**
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}.
+ *
+ * @param visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
+ * @param useGlobalMethodCacheIf 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 ReflectiveVisitDispatcher createDispatcher(
+ final Class visitorBaseClazz,
+ final Class visiteeBaseClazz,
+ final boolean useGlobalMethodCache) {
Review comment:
To maintain backward compatibility, I add one param boolean
`useGlobalMethodCache` to method `ReflectiveVisitDispatcher
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:
us...@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
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 visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
* @return cache of methods
*/
public static ReflectiveVisitDispatcher createDispatcher(
final Class visitorBaseClazz,
final Class visiteeBaseClazz) {
+return createDispatcher(visitorBaseClazz, visiteeBaseClazz, true);
+ }
+
+ /**
+ * Creates a dispatcher for calls to {@link #lookupVisitMethod}.
+ *
+ * @param visitorBaseClazzVisitor base class
+ * @param visiteeBaseClazzVisitee base class
+ * @param useGlobalMethodCacheIf 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 ReflectiveVisitDispatcher createDispatcher(
+ final Class visitorBaseClazz,
+ final Class visiteeBaseClazz,
+ final boolean useGlobalMethodCache) {
Review comment:
To maintain backward compatibility, I add one param boolean
`useGlobalMethodCache` to method `ReflectiveVisitDispatcher
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:
us...@infra.apache.org
With regards,
Apache Git Services
