You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by mi...@apache.org on 2017/12/18 17:55:56 UTC
[3/6] impala git commit: IMPALA-6284: Mark the intermediate decimal
avg struct as packed
IMPALA-6284: Mark the intermediate decimal avg struct as packed
We saw some failures on the exhaustive release build because the
compiler assumed that the pointer to the intermediate struct that is
used for computing decimal average was aligned.
To fix the problem, we mark the struct with a "packed" attribute so
that the compiler does not expect it to be aligned.
Testing:
- Ran the failing test locally on an release build and it passed.
Change-Id: Id25ec6e20dde3f50fb37a22135b355ad251809e0
Reviewed-on: http://gerrit.cloudera.org:8080/8836
Reviewed-by: Taras Bobrovytsky <tb...@cloudera.com>
Tested-by: Impala Public Jenkins
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/7256fcef
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/7256fcef
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/7256fcef
Branch: refs/heads/master
Commit: 7256fcefb4c64d3d61d88091d6447317654bd96e
Parents: 3f1f706
Author: Taras Bobrovytsky <tb...@cloudera.com>
Authored: Wed Dec 13 17:56:51 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Sat Dec 16 03:26:43 2017 +0000
----------------------------------------------------------------------
be/src/exprs/aggregate-functions-ir.cc | 6 +++++-
fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java | 2 +-
.../functional-query/queries/QueryTest/functions-ddl.test | 2 +-
3 files changed, 7 insertions(+), 3 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/7256fcef/be/src/exprs/aggregate-functions-ir.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/aggregate-functions-ir.cc b/be/src/exprs/aggregate-functions-ir.cc
index 499188c..40c0c16 100644
--- a/be/src/exprs/aggregate-functions-ir.cc
+++ b/be/src/exprs/aggregate-functions-ir.cc
@@ -374,7 +374,11 @@ TimestampVal AggregateFunctions::TimestampAvgFinalize(FunctionContext* ctx,
return result;
}
-struct DecimalAvgState {
+// We saw some failures on the release build because GCC was emitting an instruction
+// to operate on the int128_t that assumed the pointer was aligned (typically it isn't).
+// We mark the struct with a "packed" attribute, so that the compiler does not expect it
+// to be aligned. This should not have a negative performance impact on modern CPUs.
+struct __attribute__ ((__packed__)) DecimalAvgState {
__int128_t sum_val16; // Always uses max precision decimal.
int64_t count;
};
http://git-wip-us.apache.org/repos/asf/impala/blob/7256fcef/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java b/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
index f316410..f8a6b89 100644
--- a/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
+++ b/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
@@ -40,7 +40,7 @@ public class BuiltinsDb extends Db {
private static final int AVG_INTERMEDIATE_SIZE = 16;
// Size in bytes of DecimalAvgState used for decimal avg().
- private static final int DECIMAL_AVG_INTERMEDIATE_SIZE = 32;
+ private static final int DECIMAL_AVG_INTERMEDIATE_SIZE = 24;
// Size in bytes of KnuthVarianceState used for stddev(), variance(), etc.
private static final int STDDEV_INTERMEDIATE_SIZE = 24;
http://git-wip-us.apache.org/repos/asf/impala/blob/7256fcef/testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test b/testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test
index f629b37..22d7122 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/functions-ddl.test
@@ -121,7 +121,7 @@ show create aggregate function _impala_builtins.avg
FINALIZE_FN='_ZN6impala18AggregateFunctions11AvgFinalizeEPN10impala_udf15FunctionContextERKNS1_9StringValE'
CREATE AGGREGATE FUNCTION _impala_builtins.avg(DECIMAL(*,*))
RETURNS DECIMAL(*,*)
- INTERMEDIATE FIXED_UDA_INTERMEDIATE(32)
+ INTERMEDIATE FIXED_UDA_INTERMEDIATE(24)
LOCATION 'null'
UPDATE_FN='_ZN6impala18AggregateFunctions16DecimalAvgUpdateEPN10impala_udf15FunctionContextERKNS1_10DecimalValEPNS1_9StringValE'
INIT_FN='_ZN6impala18AggregateFunctions14DecimalAvgInitEPN10impala_udf15FunctionContextEPNS1_9StringValE'