[
https://issues.apache.org/jira/browse/IMPALA-4658?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16837676#comment-16837676
]
ASF subversion and git services commented on IMPALA-4658:
---------------------------------------------------------
Commit 832c9de7810b47b5f782bccb761e07264e7548e5 in impala's branch
refs/heads/master from Abhishek Rawat
[ https://gitbox.apache.org/repos/asf?p=impala.git;h=832c9de ]
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
Reviewed-on: http://gerrit.cloudera.org:8080/13178
Reviewed-by: Impala Public Jenkins <[email protected]>
Tested-by: Impala Public Jenkins <[email protected]>
> Potential race if compiler reorders ReachedLimit() usage
> --------------------------------------------------------
>
> Key: IMPALA-4658
> URL: https://issues.apache.org/jira/browse/IMPALA-4658
> Project: IMPALA
> Issue Type: Bug
> Components: Backend
> Affects Versions: Impala 2.8.0
> Reporter: Matthew Jacobs
> Assignee: Abhishek Rawat
> Priority: Major
>
> There is a potential race in our many usages of {{ExecNode::ReachedLimit()}}
> during execution from a different thread than the one updating rows_returned,
> e.g. scanner threads. The compiler in theory is free to notice that they're
> not updating {{rows_returned_}} and hoist the load up out of loops, etc.
> We should consider AtomicInt64 or NoBarrier_Load(...).
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
