Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/18861 )
Change subject: IMPALA-11504:Specializing DecimalUtil::GetScaleMultiplier<int256_t>(). ...................................................................... Patch Set 3: (14 comments) 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 careful to run the benchmark using a release build. 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: while (n-- > 0) { "for (int i = 0; i < n; ++n)" would be more readable in my opinion. http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/benchmarks/decimal-util-benchmark.cc@31 PS3, Line 31: sacle Nit: scale. http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/benchmarks/decimal-util-benchmark.cc@32 PS3, Line 32: int constexpr int? http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/benchmarks/decimal-util-benchmark.cc@33 PS3, Line 33: n We shadow the input parameter 'n' here, it would be better to choose a different variable name. http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/benchmarks/decimal-util-benchmark.cc@51 PS3, Line 51: GetScaleMultiplier(j); If we don't use the result, the compiler will probably optimise out the call entirely. We should use the result in some way that is also cheap so it doesn't change the running time much. For example we can try summing the results and at the end of the function write it to some volatile (global) variable. But we need to make sure that overflow is defined, i.e. don't use a signed type to store the sum. http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/benchmarks/decimal-util-benchmark.cc@69 PS3, Line 69: 1000 If you get errors complaining about context switches (you may get them after doing what my comment on L51 says), you can lower this number. The idea is that one call to the benchmarking function should complete without context switches but if it is too long it will not be possible. 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: GetScaleMultiplier We could add unit tests in decimal-test.cc for GetScaleMultiplier(). http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h@262 PS3, Line 262: inline int256_t DecimalUtil::GetScaleMultiplier<int256_t>(int scale) { I've got a few suggestions that also apply to the other specialisations of GetScaleMultiplier(). Now that we touch this file they could also be updated. http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h@264 PS3, Line 264: const Could be constexpr. http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h@264 PS3, Line 264: static const int256_t values[] = { The size of the array could be specified explicitly, for example: static constexpr int num_values = 77; static constexpr int256_t values[num_values] = ... http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h@342 PS3, Line 342: DCHECK_GE Could be a static_assert. http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h@342 PS3, Line 342: sizeof(values) / sizeof(int256_t) Could use 'num_values' instead. http://gerrit.cloudera.org:8080/#/c/18861/3/be/src/util/decimal-util.h@343 PS3, Line 343: 77 Could use 'num_values' instead. -- 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: 3 Gerrit-Owner: Xiang Yang <yx91...@126.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Mon, 26 Sep 2022 10:38:38 +0000 Gerrit-HasComments: Yes
