zabetak commented on code in PR #2834:
URL: https://github.com/apache/calcite/pull/2834#discussion_r937520689
##########
ubenchmark/src/jmh/java/org/apache/calcite/adapter/enumerable/CodeGenerationBenchmark.java:
##########
@@ -183,23 +184,25 @@ public void setup() {
info.javaCode =
Expressions.toString(info.classExpr.memberDeclarations, "\n",
false);
- ICompilerFactory compilerFactory;
- try {
- compilerFactory = CompilerFactoryFactory.getDefaultCompilerFactory(
- CodeGenerationBenchmark.class.getClassLoader());
- } catch (Exception e) {
- throw new IllegalStateException(
- "Unable to instantiate java compiler", e);
- }
- IClassBodyEvaluator cbe = compilerFactory.newClassBodyEvaluator();
- cbe.setClassName(info.classExpr.name);
- cbe.setExtendedClass(Utilities.class);
- cbe.setImplementedInterfaces(
- plan.getRowType().getFieldCount() == 1
- ? new Class[]{Bindable.class, Typed.class}
- : new Class[]{ArrayBindable.class});
-
cbe.setParentClassLoader(EnumerableInterpretable.class.getClassLoader());
- info.cbe = cbe;
+ info.cbeSupplier = () -> {
+ ICompilerFactory compilerFactory;
+ try {
+ compilerFactory = CompilerFactoryFactory.getDefaultCompilerFactory(
+ CodeGenerationBenchmark.class.getClassLoader());
+ } catch (Exception e) {
+ throw new IllegalStateException(
+ "Unable to instantiate java compiler", e);
+ }
Review Comment:
Do we also need to include the `compilerFactory` in the `Supplier`? Whatever
we include in the supplier becomes part of the benchmark (and not part of the
setup) so we should reduce it to the minimal.
##########
ubenchmark/src/jmh/java/org/apache/calcite/adapter/enumerable/CodeGenerationBenchmark.java:
##########
@@ -183,23 +184,25 @@ public void setup() {
info.javaCode =
Expressions.toString(info.classExpr.memberDeclarations, "\n",
false);
- ICompilerFactory compilerFactory;
- try {
- compilerFactory = CompilerFactoryFactory.getDefaultCompilerFactory(
- CodeGenerationBenchmark.class.getClassLoader());
- } catch (Exception e) {
- throw new IllegalStateException(
- "Unable to instantiate java compiler", e);
- }
- IClassBodyEvaluator cbe = compilerFactory.newClassBodyEvaluator();
- cbe.setClassName(info.classExpr.name);
- cbe.setExtendedClass(Utilities.class);
- cbe.setImplementedInterfaces(
- plan.getRowType().getFieldCount() == 1
- ? new Class[]{Bindable.class, Typed.class}
- : new Class[]{ArrayBindable.class});
-
cbe.setParentClassLoader(EnumerableInterpretable.class.getClassLoader());
- info.cbe = cbe;
+ info.cbeSupplier = () -> {
+ ICompilerFactory compilerFactory;
+ try {
+ compilerFactory = CompilerFactoryFactory.getDefaultCompilerFactory(
+ CodeGenerationBenchmark.class.getClassLoader());
+ } catch (Exception e) {
+ throw new IllegalStateException(
+ "Unable to instantiate java compiler", e);
+ }
+ IClassBodyEvaluator cbe = compilerFactory.newClassBodyEvaluator();
+ cbe.setClassName(info.classExpr.name);
+ cbe.setExtendedClass(Utilities.class);
+ cbe.setImplementedInterfaces(
+ plan.getRowType().getFieldCount() == 1
+ ? new Class[]{Bindable.class, Typed.class}
+ : new Class[]{ArrayBindable.class});
+
cbe.setParentClassLoader(EnumerableInterpretable.class.getClassLoader());
Review Comment:
Since the setup of the CBE is now becoming part of the benchmark maybe it
would make more sense to extract it to a method and call it directly in the
benchmark methods but I don't have strong feelings about this. Keeping it in a
supplier should be fine 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
