Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 )
Change subject: IMPALA-10950: Update expr-benchmark.cc ...................................................................... Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/17894/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17894/4//COMMIT_MSG@19 PS4, Line 19: ru > nit. remove? Done http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@141 PS3, Line 141: private: : Frontend frontend_; : ExecEnv exec_env_; : > Since ScalarExpEvaluator now maintain resources, I was thinking that we sho Actually, I decide to remove this in patch set 5. http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc File be/src/benchmarks/expr-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@51 PS4, Line 51: // Struct holding reference to RuntimeState, FragmentState, ScalarExpr, and > May need to add some description on this struct. TestData may be named as E Done http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@52 PS4, Line 52: / ScalarExprEvaluator for a specific expression. : // This ExprTestData should be obtained through Planner::PrepareScalarExpression() that : // will populate all the objects and open the ScalarExprEvaluator. : struct ExprTestData { > Should define a constructor to init these data members. Done http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@81 PS4, Line 81: public: > nit. May add comment: *test_data contains valid TestData in which expr is a Done http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@84 PS4, Line 84: ec_env_.In > I wonder if there is even a need to have this vector. Removed. http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@87 PS4, Line 87: : // Tell planner to enable/disable codegen on PrepareScalarExpression. : void SetCodegen(bool enable) { query_options_.__set_disable_codegen(!enable); } : : // Create ExprTestData for a given query. > nit. May use TQueryCtx cstr to populate query_ctx. I think TQueryCtx only has empty constructor. But I tidy this up a bit so it is shorter. Hope it looks better. http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@96 PS4, Line 96: TQueryCtx q > Better to prepare these arguments first and then call the construct with th Done http://gerrit.cloudera.org:8080/#/c/17894/4/be/src/benchmarks/expr-benchmark.cc@710 PS4, Line 710: 173 > I wonder why the numbers in 10%ile, 50%ile and 90%ile column are larger in The benchmark measure how many iters/ms can be done to evaluate a ScalarExprEvaluator 256 times (this is driven by BenchmarkQueryFn). The codegen version often has much higher iters/ms because it is faster. I think it does not have reinterpret_cast and the function call path is shorter that the non-codegen. -- To view, visit http://gerrit.cloudera.org:8080/17894 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5b17434d85e32a58622bffb64a697b062a8bf43f Gerrit-Change-Number: 17894 Gerrit-PatchSet: 5 Gerrit-Owner: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Wed, 06 Oct 2021 21:44:30 +0000 Gerrit-HasComments: Yes
