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 <tbobrovytsky@cloudera.com>
Tested-by: Impala Public Jenkins
(cherry picked from commit 7256fcefb4c64d3d61d88091d6447317654bd96e)
diff --git a/be/src/exprs/aggregate-functions-ir.cc b/be/src/exprs/aggregate-functions-ir.cc
index d5f946a..bf4446d 100644
--- a/be/src/exprs/aggregate-functions-ir.cc
+++ b/be/src/exprs/aggregate-functions-ir.cc
@@ -372,7 +372,11 @@
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;
};
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 07699d3..e603fc6 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 @@
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;
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 @@
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'
