Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/17894 )
Change subject: IMPALA-10950: Update expr-benchmark.cc ...................................................................... Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG@9 PS3, Line 9: With the introduction of PlanRootSink by IMPALA-2905, query planner has : moved the scalar expression's thrift definition from : 'fragments[0].output_sink.output_exprs' to > which change made this switch? curious as to what the context there was This change was initiated by IMPALA-2905 by adding PlanRootSink. I add this explanation in the commit message. http://gerrit.cloudera.org:8080/#/c/17894/3//COMMIT_MSG@16 PS3, Line 16: sion rewrite by : FoldConstantsRule.java. > why are they now crashing if the benchmark earlier used to run fine without My bad. I'm not aware that ScalarExprEvaluator::Open() need to be called first. Otherwise, its fn_ctx->impl()->staging_input_vals() will be empty and crash the interpreted function call path. Patch set 4 fix this. 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@108 PS3, Line 108: vector<TExpr> texprs = union_node.const_expr_lists[0]; > you can use this to query option to disable re-write: ENABLE_EXPR_REWRITES You're absolutely right! We explicitly set ENABLE_EXPR_REWRITES=0 in patch set 4 to avoid expression rewrite. http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@119 PS3, Line 119: RETURN_IF_ERROR(test->fragment_state->CreateCodegen()); : LlvmCodeGen* codegen = test->fragment_state->codegen(); : DCHECK(codegen != NULL); : RETURN_IF_ERROR(test-> > do these need to be removed? Done http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@134 PS3, Line 134: ol_.Clear(); > why are we disabling this? Sorry, this was blindly copied from fe-support.cc. Patch set 4 enable the optimization. http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@141 PS3, Line 141: vector<unique_ptr<TestData>> test_data_; : : TQueryOptions query_options_; : T > nit: you can probably just add this to the destructor instead of a separate Since ScalarExpEvaluator now maintain resources, I was thinking that we should clean up all dangling resources once a benchmark suite measurement has finished. Patch set 4 add another call to this function right after Beanchmark::Measure() call. I tried run the benchmark with and without calling ReleaseTestData() after measurement. The measurement result between the two, however, does not seem to differ much. But considering we're running more benchmark suite and expressions, I think cleaning up after measurement is the right thing to do. Please let me know what you think. http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@183 PS3, Line 183: data->dummy_result += reinterpret_cast<int64_t>(value); : } > we should probably do both with and without codegen and also keep the bench Done. Controlled through Planner::SetCodegen(). http://gerrit.cloudera.org:8080/#/c/17894/3/be/src/benchmarks/expr-benchmark.cc@234 PS3, Line 234: // (relative) (relative) (relative) : // --------------------------------------------------------------------------------------------------------- : // equals 254 255 257 1X 1X 1X : // not equals 320 322 324 1.26X 1.26X 1.26X : // strstr 106 107 107 0.419X 0.419X 0.418X : // strncmp1 215 216 217 0.848X 0.847X 0.845X : // strncmp2 209 211 211 0.825X 0.825X 0.822X : // strncmp3 254 256 257 1X 1X 1X : // regex 28.1 28.2 28.3 0.111X 0.11X 0.11X : // : // LikeCodegen: Function iters/ms 10%ile 50%ile 90%ile 10%ile 50%ile 90%ile : // > all of these values are for benchmark without codegen, can you update these Done -- 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: 4 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: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Comment-Date: Wed, 06 Oct 2021 06:07:39 +0000 Gerrit-HasComments: Yes
