Abhishek Rawat has uploaded this change for review. ( 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 a new atomic member ExecNode::num_rows_returned_shared_, to be used in multithreaded code paths. Both the num_rows_returned_ and num_rows_returned_shared_ members are now private to the ExecNode class. The subclasses must access/update them through the public getters/modifiers. The getters/ modifiers for single threaded code-path are: rows_returned(), ReachedLimit(), SetNumRowsReturned(), IncrementNumRowsReturned(), and DecrementNumRowsReturned(). Thread safe counterparts for these functions are: rows_returned_shared(), ReachedLimitShared(), IncrementNumRowsReturnedShared(), and DecrementNumRowsReturnedShared(). Debug checks were added to ensure that num_rows_returned_ is used in single threaded code-path and num_rows_returned_shared_ is used in multi threaded code path. 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 variables 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, even though the variable is not shared amongst various scan node threads. This is because both the task based MT code path and 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. 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 change. 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.cc 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.cc 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 32 files changed, 168 insertions(+), 111 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/78/13178/2 -- 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: newchange Gerrit-Change-Id: I4cbbfad80f7ab87dd6f192a24e2c68f7c66b047e Gerrit-Change-Number: 13178 Gerrit-PatchSet: 2 Gerrit-Owner: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]>
