impala-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From tmarsh...@apache.org
Subject impala git commit: IMPALA-6284: Mark the intermediate decimal avg struct as packed
Date Tue, 19 Dec 2017 22:07:39 GMT
Repository: impala
Updated Branches:
  refs/heads/branch-2.11.0 178b00ee2 -> b49d14934


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)


Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/b49d1493
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/b49d1493
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/b49d1493

Branch: refs/heads/branch-2.11.0
Commit: b49d149343c7ea0700e41cda3bdc2efcb8ea7e6d
Parents: 178b00e
Author: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Authored: Wed Dec 13 17:56:51 2017 -0800
Committer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Committed: Mon Dec 18 10:05:16 2017 -0800

----------------------------------------------------------------------
 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/b49d1493/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 d5f946a..bf4446d 100644
--- a/be/src/exprs/aggregate-functions-ir.cc
+++ b/be/src/exprs/aggregate-functions-ir.cc
@@ -372,7 +372,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/b49d1493/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 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 @@ 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/b49d1493/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'


Mime
View raw message