Xiang Yang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18861 )
Change subject: IMPALA-11504:Specializing DecimalUtil::GetScaleMultiplier<int256_t>(). ...................................................................... Patch Set 4: (14 comments) Hi Daniel, thanks for your suggestions! I still have 2 remain questions, waiting for your reply. http://gerrit.cloudera.org:8080/#/c/18861/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18861/3//COMMIT_MSG@15 PS3, Line 15: - Add decimal-util-benchmark. > You could include the result of the benchmark in the commit message. Be car done, but ran in a docker container, will that be a problem? http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/benchmarks/decimal-util-benchmark.cc File be/src/benchmarks/decimal-util-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/benchmarks/decimal-util-benchmark.cc@30 PS3, Line 30: // raw 0.704 0.704 0.717 1X 1X 1X > "for (int i = 0; i < n; ++n)" would be more readable in my opinion. Done http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/benchmarks/decimal-util-benchmark.cc@31 PS3, Line 31: > Nit: scale. Done http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/benchmarks/decimal-util-benchmark.cc@32 PS3, Line 32: > constexpr int? Done http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/benchmarks/decimal-util-benchmark.cc@33 PS3, Line 33: m > We shadow the input parameter 'n' here, it would be better to choose a diff Done http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/benchmarks/decimal-util-benchmark.cc@51 PS3, Line 51: } > If we don't use the result, the compiler will probably optimise out the cal done. But I found another approach : https://stackoverflow.com/questions/7083482#answer-58758133. that is add '__asm__ volatile("" : "+g" (m) : :);' after GetScaleMultiplier(). the result is 1400X, which has big difference with current. http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/benchmarks/decimal-util-benchmark.cc@69 PS3, Line 69: <int > If you get errors complaining about context switches (you may get them afte I didn't meet this situation. http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h File be/src/util/decimal-util.h: http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h@262 PS3, Line 262: 00ll, > We could add unit tests in decimal-test.cc for GetScaleMultiplier(). Done http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h@262 PS3, Line 262: i10e18 * 1000000000000000ll, > I've got a few suggestions that also apply to the other specialisations of Done http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h@264 PS3, Line 264: e18 * > Could be constexpr. Done http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h@264 PS3, Line 264: i10e18 * 100000000000000000ll, > The size of the array could be specified explicitly, for example: Done http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h@342 PS3, Line 342: i10e1 > Could be a static_assert. Done http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h@342 PS3, Line 342: * i10e18 * i10e18 * 1000000000ll > Could use 'num_values' instead. use 'num_values' instead of "sizeof(values) / sizeof(int256_t)"? it seems that the whole assert statement is meaningless. http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h@343 PS3, Line 343: * > Could use 'num_values' instead. Done -- To view, visit http://gerrit.cloudera.org:8080/18861 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I969e2977d51313e738f72c8246db003ae43a3782 Gerrit-Change-Number: 18861 Gerrit-PatchSet: 4 Gerrit-Owner: Xiang Yang <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Xiang Yang <[email protected]> Gerrit-Comment-Date: Tue, 27 Sep 2022 07:57:30 +0000 Gerrit-HasComments: Yes
