Abhishek Rawat has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/13178 )
Change subject: IMPALA-4658: Potential race if compiler reorders ReachedLimit() usage. ...................................................................... IMPALA-4658: Potential race if compiler reorders ReachedLimit() usage. The ExecNode::num_rows_returned_ member is shared by the scan node (thread) and the related scanner threads. There is a potential race here since the compiler is free to reorder the load/store of num_rows_returned_ member. Also, num_rows_returned_ is not accessed in a thread safe manner. This change introduces regular and thread safe functions for accessing and updating the num_rows_returned_ member, which is now private to the ExecNode class. The getters/modifiers for single threaded code-paths are: rows_returned() ReachedLimit() SetNumRowsReturned() IncrementNumRowsReturned() DecrementNumRowsReturned() CheckLimitAndTruncateRowBatchIfNeeded() Thread safe counterparts are: rows_returned_shared() ReachedLimitShared() IncrementNumRowsReturnedShared() DecrementNumRowsReturnedShared() CheckLimitAndTruncateRowBatchIfNeededShared() Debug checks are added to ensure that regular and thread safe functions are used properly in single-threaded and multi-threaded code paths, respectively. The initial attempt to address this issue by making num_rows_returned_ atomic caused performance regressions in single threaded code paths which update the variable inside a loop, such as ExchangeNode::GetNext(). The regression could be attributed to extra path length introduced by atomic operations such as read modify write. And since the GetNext() function could be called multiple times, with each call updating the atomic multiple times, the regression could be huge. Some serious thoughts were given to other approaches such as updating the atomic num_rows_returned_ variable outside of a loop, but such a change would have been very extensive and error prone. There is nothing stopping someone from not adding atomic updates in a tight loop. After much deliberation it was decided that the single threaded code path should not be penalised and as such it probably makes sense to use separate functions for single threaded and multi-threaded code paths, provided the interfaces are simple enough, easy to maintain and there are checks in place to ensure proper use. The scan nodes with task based multi-threading (MT) also use the new thread-safe functions in the code-paths shared with the non MT scan nodes, even though the variable is not shared amongst various threads. This is because both the task based MT code-path and the regular scan node code path calls some common functions and it was getting very tricky and ugly to differentiate between the two. The overhead of using atomics is minimal for MT code path since the atomic is not accessed for every row. The only exceptions are HBaseScanNode and DataSourceScanNode which uses the regular functions. Testing: - Performance tests were run on tpch and tpcds workload. No real regressions were found, but both baseline and test runs showed high degree of variability for some queries. Multiple runs were done and it was concluded that the variability is not introduced by this patch. Change-Id: I4cbbfad80f7ab87dd6f192a24e2c68f7c66b047e --- M be/src/exec/aggregation-node.cc M be/src/exec/analytic-eval-node.cc M be/src/exec/cardinality-check-node.cc M be/src/exec/data-source-scan-node.cc M be/src/exec/exchange-node.cc M be/src/exec/exec-node.cc M be/src/exec/exec-node.h M be/src/exec/hbase-scan-node.cc M be/src/exec/hdfs-avro-scanner.cc M be/src/exec/hdfs-orc-scanner.cc M be/src/exec/hdfs-rcfile-scanner.cc M be/src/exec/hdfs-scan-node-base.cc M be/src/exec/hdfs-scan-node-mt.cc M be/src/exec/hdfs-scan-node-mt.h M be/src/exec/hdfs-scan-node.cc M be/src/exec/hdfs-scan-node.h M be/src/exec/hdfs-scanner.cc M be/src/exec/hdfs-sequence-scanner.cc M be/src/exec/hdfs-text-scanner.cc M be/src/exec/kudu-scan-node-mt.cc M be/src/exec/kudu-scan-node-mt.h M be/src/exec/kudu-scan-node.cc M be/src/exec/kudu-scan-node.h M be/src/exec/kudu-scanner.cc M be/src/exec/nested-loop-join-node.cc M be/src/exec/parquet/hdfs-parquet-scanner.cc M be/src/exec/partial-sort-node.cc M be/src/exec/partitioned-hash-join-node.cc M be/src/exec/select-node-ir.cc M be/src/exec/select-node.cc M be/src/exec/sort-node.cc M be/src/exec/streaming-aggregation-node.cc M be/src/exec/subplan-node.cc M be/src/exec/topn-node.cc M be/src/exec/union-node.cc M be/src/exec/unnest-node.cc 36 files changed, 213 insertions(+), 133 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/13178/5 -- To view, visit http://gerrit.cloudera.org:8080/13178 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4cbbfad80f7ab87dd6f192a24e2c68f7c66b047e Gerrit-Change-Number: 13178 Gerrit-PatchSet: 5 Gerrit-Owner: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
