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:
us...@infra.apache.org
With regards,
Apache Git Services
