You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/06/30 16:27:39 UTC
[GitHub] [arrow] pitrou opened a new pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
pitrou opened a new pull request #10632:
URL: https://github.com/apache/arrow/pull/10632
Also ensure that the llvm-symbolizer path is correctly set, for useful tracebacks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r662207074
##########
File path: cpp/src/arrow/compute/exec/expression.h
##########
@@ -131,15 +132,15 @@ inline bool operator!=(const Expression& l, const Expression& r) { return !l.Equ
// Factories
ARROW_EXPORT
-Expression literal(Datum lit);
+Expression literal(Datum&& lit);
Review comment:
Please revert this; it's not illegal for these factories to accept a copy:
```c++
const Datum& copyable = GetDatumRefFromSomewhere();
Expression expr = literal(copyable); // will fail to compile if literal() only accepts Datum&&
```
##########
File path: cpp/src/arrow/compute/exec/expression.cc
##########
@@ -42,18 +42,17 @@ using internal::checked_pointer_cast;
namespace compute {
-Expression::Expression(Call call) : impl_(std::make_shared<Impl>(std::move(call))) {}
+Expression::Expression(Call&& call) : impl_(std::make_shared<Impl>(call)) {}
Review comment:
A declaration of `Call&& call` doesn't get automatically moved on the first reference expression, so we still need the call to `std::move`
```suggestion
Expression::Expression(Call&& call) : impl_(std::make_shared<Impl>(std::move(call))) {}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-871573504
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pachadotdev commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pachadotdev commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-872595668
@github-actions crossbow submit test-r-linux-valgrind
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r662218448
##########
File path: cpp/src/arrow/util/async_generator_test.cc
##########
@@ -67,14 +68,14 @@ class TrackingGenerator {
return state_->source();
}
- int num_read() { return state_->num_read; }
+ int num_read() { return state_->num_read.load(); }
private:
struct State {
explicit State(AsyncGenerator<T> source) : source(std::move(source)), num_read(0) {}
AsyncGenerator<T> source;
- int num_read;
+ std::atomic<int> num_read;
Review comment:
@westonpace I had to change this as well, got a sporadic race condition in `MergedGeneratorTests/MergedGeneratorTestFixture.MergedLimitedSubscriptions/1`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r662129264
##########
File path: cpp/src/arrow/compute/exec/expression.cc
##########
@@ -294,19 +293,21 @@ size_t Expression::hash() const {
}
auto call = CallNotNull(*this);
- if (call->hash != nullptr) {
- return call->hash->load();
+ const auto cached_hash = call->hash.load();
Review comment:
Sure, we can if the cost is not annoying.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-871553967
https://issues.apache.org/jira/browse/ARROW-13223
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r661635708
##########
File path: cpp/src/arrow/util/async_generator.h
##########
@@ -1338,11 +1342,34 @@ class BackgroundGenerator {
return next;
}
+#if (defined(__GNUC__) || defined(__clang__))
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wclass-memaccess"
+#endif
+
+ void SetWorkerThreadId(const std::thread::id tid) {
Review comment:
Of course, we could also not bother with this complication, since `worker_thread_id` is only used for debugging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-871555267
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-871573981
Revision: 2b062b03a19ab291ee3401d00333923a6ebaa29f
Submitted crossbow builds: [ursacomputing/crossbow @ actions-538](https://github.com/ursacomputing/crossbow/branches/all?query=actions-538)
|Task|Status|
|----|------|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-538-github-test-conda-cpp)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-538-github-test-conda-cpp-valgrind)|
|test-debian-10-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-538-github-test-debian-10-cpp)|
|test-fedora-33-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-538-github-test-fedora-33-cpp)|
|test-ubuntu-18.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-538-github-test-ubuntu-18.04-cpp)|
|test-ubuntu-18.04-cpp-release|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-538-github-test-ubuntu-18.04-cpp-release)|
|test-ubuntu-18.04-cpp-static|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-538-github-test-ubuntu-18.04-cpp-static)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-538-github-test-ubuntu-20.04-cpp)|
|test-ubuntu-20.04-cpp-14|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-538-github-test-ubuntu-20.04-cpp-14)|
|test-ubuntu-20.04-cpp-17|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-538-github-test-ubuntu-20.04-cpp-17)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-538-github-test-ubuntu-20.04-cpp-thread-sanitizer)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-872177721
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r662570695
##########
File path: cpp/src/arrow/util/async_generator.h
##########
@@ -1338,11 +1342,34 @@ class BackgroundGenerator {
return next;
}
+#if (defined(__GNUC__) || defined(__clang__))
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wclass-memaccess"
+#endif
+
+ void SetWorkerThreadId(const std::thread::id tid) {
Review comment:
See https://github.com/apache/arrow/pull/10644
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r661635069
##########
File path: dev/tasks/tasks.yml
##########
@@ -824,7 +824,10 @@ tasks:
template: docker-tests/github.linux.yml
params:
env:
+ # clang-tools and llvm version need to be synchronized so as
+ # to have the right llvm-symbolizer version
CLANG_TOOLS: 11
+ LLVM: 11
Review comment:
@kszucs Does this seem ok?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-872216650
Rebased and fixed conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-872126471
Revision: bc4f23f319ad7922f5a3debb9411d0f6e36fad27
Submitted crossbow builds: [ursacomputing/crossbow @ actions-542](https://github.com/ursacomputing/crossbow/branches/all?query=actions-542)
|Task|Status|
|----|------|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-542-github-test-conda-cpp)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-542-github-test-conda-cpp-valgrind)|
|test-debian-10-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-542-github-test-debian-10-cpp)|
|test-fedora-33-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-542-github-test-fedora-33-cpp)|
|test-ubuntu-18.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-542-github-test-ubuntu-18.04-cpp)|
|test-ubuntu-18.04-cpp-release|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-542-github-test-ubuntu-18.04-cpp-release)|
|test-ubuntu-18.04-cpp-static|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-542-github-test-ubuntu-18.04-cpp-static)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-542-github-test-ubuntu-20.04-cpp)|
|test-ubuntu-20.04-cpp-14|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-542-github-test-ubuntu-20.04-cpp-14)|
|test-ubuntu-20.04-cpp-17|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-542-github-test-ubuntu-20.04-cpp-17)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-542-github-test-ubuntu-20.04-cpp-thread-sanitizer)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #10632:
URL: https://github.com/apache/arrow/pull/10632
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r662218951
##########
File path: cpp/src/arrow/compute/exec/expression.cc
##########
@@ -62,6 +62,11 @@ Expression call(std::string function, std::vector<Expression> arguments,
call.function_name = std::move(function);
call.arguments = std::move(arguments);
call.options = std::move(options);
+
+ call.hash = std::hash<std::string>{}(call.function_name);
Review comment:
Hmmm, the Valgrind failure should be fixed if we move computation of the hash into `Expresion::Expression(Call)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r661709398
##########
File path: cpp/src/arrow/util/async_generator.h
##########
@@ -1338,11 +1342,34 @@ class BackgroundGenerator {
return next;
}
+#if (defined(__GNUC__) || defined(__clang__))
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wclass-memaccess"
+#endif
+
+ void SetWorkerThreadId(const std::thread::id tid) {
Review comment:
True, but the assert catches an otherwise potentially subtle issue in how BackgroundGenerator is meant to be used. It's not entirely obvious that you aren't allowed to dispose of it from the worker thread and it a fair amount of debugging to discover this root cause.
What was the issue? Is it because the worker thread id might be changing (from set to unset) when the assert in cleanup happens?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-872217031
Revision: 92579323217eb81a410450de5cddc8df6a47cb9e
Submitted crossbow builds: [ursacomputing/crossbow @ actions-545](https://github.com/ursacomputing/crossbow/branches/all?query=actions-545)
|Task|Status|
|----|------|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-545-github-test-conda-cpp)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-545-github-test-conda-cpp-valgrind)|
|test-debian-10-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-545-github-test-debian-10-cpp)|
|test-fedora-33-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-545-github-test-fedora-33-cpp)|
|test-ubuntu-18.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-545-github-test-ubuntu-18.04-cpp)|
|test-ubuntu-18.04-cpp-release|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-545-github-test-ubuntu-18.04-cpp-release)|
|test-ubuntu-18.04-cpp-static|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-545-github-test-ubuntu-18.04-cpp-static)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-545-github-test-ubuntu-20.04-cpp)|
|test-ubuntu-20.04-cpp-14|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-545-github-test-ubuntu-20.04-cpp-14)|
|test-ubuntu-20.04-cpp-17|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-545-github-test-ubuntu-20.04-cpp-17)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-545-github-test-ubuntu-20.04-cpp-thread-sanitizer)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r662172979
##########
File path: cpp/src/arrow/util/async_generator.h
##########
@@ -1338,11 +1342,34 @@ class BackgroundGenerator {
return next;
}
+#if (defined(__GNUC__) || defined(__clang__))
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wclass-memaccess"
+#endif
+
+ void SetWorkerThreadId(const std::thread::id tid) {
Review comment:
> Is it because the worker thread id might be changing (from set to unset) when the assert in cleanup happens?
Exactly, yes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
westonpace commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r662529376
##########
File path: cpp/src/arrow/util/async_generator.h
##########
@@ -1338,11 +1342,34 @@ class BackgroundGenerator {
return next;
}
+#if (defined(__GNUC__) || defined(__clang__))
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wclass-memaccess"
+#endif
+
+ void SetWorkerThreadId(const std::thread::id tid) {
Review comment:
If this neat trick to turn a `thread::id` into an `int64_t` is indeed reliable it might be nice to add `int64_t arrow::internal::GetCurrentThreadId()`. Probably in `thread_pool.h/cc`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r661696226
##########
File path: cpp/src/arrow/compute/exec/expression.cc
##########
@@ -294,19 +293,21 @@ size_t Expression::hash() const {
}
auto call = CallNotNull(*this);
- if (call->hash != nullptr) {
- return call->hash->load();
+ const auto cached_hash = call->hash.load();
Review comment:
Instead, could we simply avoid lazy hashing?
https://github.com/apache/arrow/pull/10397/files?file-filters%5B%5D=.cc&file-filters%5B%5D=.cpp&file-filters%5B%5D=.h&file-filters%5B%5D=.pxd&file-filters%5B%5D=.pyx#diff-1c7baea2c704ebf6ec38232e30aa84900e7ea8f81d6faa0f138c30cd3e4ee7ecR45-R51
This will remove the need to write Call constructors too
##########
File path: cpp/src/arrow/util/async_generator.h
##########
@@ -1338,11 +1342,34 @@ class BackgroundGenerator {
return next;
}
+#if (defined(__GNUC__) || defined(__clang__))
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wclass-memaccess"
+#endif
+
+ void SetWorkerThreadId(const std::thread::id tid) {
+ uint64_t equiv{0};
+ // std::thread::id is trivially copyable as per C++ spec, so this should work
Review comment:
```suggestion
// std::thread::id is trivially copyable as per C++ spec,
// so type punning as a uint64_t should work
static_assert(sizeof(std::thread::id) <= sizeof(uint64_t),
"std::thread::id can't fit into uint64_t");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-872193671
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-872216499
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r662225815
##########
File path: cpp/src/arrow/compute/exec/expression.cc
##########
@@ -62,6 +62,11 @@ Expression call(std::string function, std::vector<Expression> arguments,
call.function_name = std::move(function);
call.arguments = std::move(arguments);
call.options = std::move(options);
+
+ call.hash = std::hash<std::string>{}(call.function_name);
Review comment:
Ah, I had missed the Valgrind failure. My bad.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r662208272
##########
File path: cpp/src/arrow/compute/exec/expression_test.cc
##########
@@ -522,7 +522,7 @@ TEST(Expression, BindNestedCall) {
TEST(Expression, ExecuteFieldRef) {
auto ExpectRefIs = [](FieldRef ref, Datum in, Datum expected) {
- auto expr = field_ref(ref);
+ auto expr = field_ref(std::move(ref));
Review comment:
This is fine, but I think it's acceptable to be sloppier with moves in tests because it makes debugging easier
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-872595980
Revision: 92579323217eb81a410450de5cddc8df6a47cb9e
Submitted crossbow builds: [ursacomputing/crossbow @ actions-557](https://github.com/ursacomputing/crossbow/branches/all?query=actions-557)
|Task|Status|
|----|------|
|test-r-linux-valgrind|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-557-azure-test-r-linux-valgrind)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r662550074
##########
File path: cpp/src/arrow/util/async_generator.h
##########
@@ -1338,11 +1342,34 @@ class BackgroundGenerator {
return next;
}
+#if (defined(__GNUC__) || defined(__clang__))
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wclass-memaccess"
+#endif
+
+ void SetWorkerThreadId(const std::thread::id tid) {
Review comment:
Will do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#discussion_r662211795
##########
File path: cpp/src/arrow/compute/exec/expression.h
##########
@@ -131,15 +132,15 @@ inline bool operator!=(const Expression& l, const Expression& r) { return !l.Equ
// Factories
ARROW_EXPORT
-Expression literal(Datum lit);
+Expression literal(Datum&& lit);
Review comment:
Oops, sorry. I think I went overboard with these changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-871555860
Revision: 6f08ec8c6125422b2d36455d604acd2a6fdefe03
Submitted crossbow builds: [ursacomputing/crossbow @ actions-537](https://github.com/ursacomputing/crossbow/branches/all?query=actions-537)
|Task|Status|
|----|------|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-537-github-test-conda-cpp)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-537-github-test-conda-cpp-valgrind)|
|test-debian-10-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-537-github-test-debian-10-cpp)|
|test-fedora-33-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-537-github-test-fedora-33-cpp)|
|test-ubuntu-18.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-537-github-test-ubuntu-18.04-cpp)|
|test-ubuntu-18.04-cpp-release|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-537-github-test-ubuntu-18.04-cpp-release)|
|test-ubuntu-18.04-cpp-static|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-537-github-test-ubuntu-18.04-cpp-static)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-537-github-test-ubuntu-20.04-cpp)|
|test-ubuntu-20.04-cpp-14|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-537-github-test-ubuntu-20.04-cpp-14)|
|test-ubuntu-20.04-cpp-17|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-537-github-test-ubuntu-20.04-cpp-17)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-537-github-test-ubuntu-20.04-cpp-thread-sanitizer)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-872121279
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-872194361
Revision: 42c3638600296c3e92b87a82962d23c7a336d578
Submitted crossbow builds: [ursacomputing/crossbow @ actions-544](https://github.com/ursacomputing/crossbow/branches/all?query=actions-544)
|Task|Status|
|----|------|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-544-github-test-conda-cpp)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-544-github-test-conda-cpp-valgrind)|
|test-debian-10-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-544-github-test-debian-10-cpp)|
|test-fedora-33-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-544-github-test-fedora-33-cpp)|
|test-ubuntu-18.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-544-github-test-ubuntu-18.04-cpp)|
|test-ubuntu-18.04-cpp-release|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-544-github-test-ubuntu-18.04-cpp-release)|
|test-ubuntu-18.04-cpp-static|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-544-github-test-ubuntu-18.04-cpp-static)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-544-github-test-ubuntu-20.04-cpp)|
|test-ubuntu-20.04-cpp-14|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-544-github-test-ubuntu-20.04-cpp-14)|
|test-ubuntu-20.04-cpp-17|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-544-github-test-ubuntu-20.04-cpp-17)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-544-github-test-ubuntu-20.04-cpp-thread-sanitizer)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10632: ARROW-13223: [C++] Fix Thread Sanitizer test failures
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10632:
URL: https://github.com/apache/arrow/pull/10632#issuecomment-872178218
Revision: 8c315e12db500692d42e3c67d078e01f336d12f8
Submitted crossbow builds: [ursacomputing/crossbow @ actions-543](https://github.com/ursacomputing/crossbow/branches/all?query=actions-543)
|Task|Status|
|----|------|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-543-github-test-conda-cpp)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-543-github-test-conda-cpp-valgrind)|
|test-debian-10-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-543-github-test-debian-10-cpp)|
|test-fedora-33-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-543-github-test-fedora-33-cpp)|
|test-ubuntu-18.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-543-github-test-ubuntu-18.04-cpp)|
|test-ubuntu-18.04-cpp-release|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-543-github-test-ubuntu-18.04-cpp-release)|
|test-ubuntu-18.04-cpp-static|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-543-github-test-ubuntu-18.04-cpp-static)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-543-github-test-ubuntu-20.04-cpp)|
|test-ubuntu-20.04-cpp-14|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-543-github-test-ubuntu-20.04-cpp-14)|
|test-ubuntu-20.04-cpp-17|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-543-github-test-ubuntu-20.04-cpp-17)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-543-github-test-ubuntu-20.04-cpp-thread-sanitizer)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org