ARROW-14211: [C++][Compute] Fixing thread sanitizer problems in hash join node #11350

michalursa · 2021-10-07T04:39:39Z

Fixing 3 issues:

one in SchemaProjectionMaps - I simplified all of the code to get rid of thread synchronization at all
one in TaskScheduler - added (unnecessary) mutex
one in HashJoinImpl - switching from shared byte vector to local bit vectors and merge (for recording if a match for a hash table row has been seen)

michalursa · 2021-10-07T04:39:56Z

@github-actions crossbow submit -g cpp

github-actions · 2021-10-07T04:39:58Z

https://issues.apache.org/jira/browse/ARROW-14197

github-actions · 2021-10-07T04:41:19Z

Revision: 16ded42dddc5e654dd177707c1c47dd6d8005c0d

Submitted crossbow builds: ursacomputing/crossbow @ actions-902

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-11-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

michalursa · 2021-10-07T05:34:16Z

@github-actions crossbow submit -g cpp

github-actions · 2021-10-07T05:35:14Z

Revision: 96823881bfd42876dfbcc577415a1870611fee25

Submitted crossbow builds: ursacomputing/crossbow @ actions-903

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-11-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

cpp/src/arrow/compute/exec/tpch_test.cc

github-actions · 2021-10-07T19:12:32Z

https://issues.apache.org/jira/browse/ARROW-14211

michalursa · 2021-10-11T00:58:23Z

@github-actions crossbow submit -g cpp

github-actions · 2021-10-11T00:59:21Z

Revision: 679309bb681dd4e40caf5cccf608a867d97f3add

Submitted crossbow builds: ursacomputing/crossbow @ actions-908

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-11-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

michalursa · 2021-10-11T10:07:09Z

@github-actions crossbow submit -g cpp

github-actions · 2021-10-11T10:14:07Z

Revision: 3c32de96b05171c126d1cf5e08ce019eaedb244f

Submitted crossbow builds: ursacomputing/crossbow @ actions-910

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-11-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

michalursa · 2021-10-11T21:44:30Z

@github-actions crossbow submit -g cpp

github-actions · 2021-10-11T21:45:34Z

Revision: f1dae79664dc0be394e0544b62ef79d7e1e34685

Submitted crossbow builds: ursacomputing/crossbow @ actions-914

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-11-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

michalursa · 2021-10-11T22:47:19Z

@github-actions crossbow submit -g cpp

github-actions · 2021-10-11T22:48:19Z

Revision: 1deb369bd9dccd27475ee47574ec14015f35b532

Submitted crossbow builds: ursacomputing/crossbow @ actions-915

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-11-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

michalursa · 2021-10-12T06:43:54Z

@github-actions crossbow submit -g cpp

github-actions · 2021-10-12T06:58:22Z

Revision: 3d1ce0c31fdab96ef969d790fc3ac673c8749379

Submitted crossbow builds: ursacomputing/crossbow @ actions-917

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-11-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

pitrou · 2021-10-14T08:41:15Z

@michalursa Is this ready for review? Ideally we would like to ship this fix in 6.0.0, which will hopefully be cut in a few days.

github-actions · 2021-10-14T08:45:12Z

Revision: c94bd41ed08dcbb3dd22585cf97fa536abc48262

Submitted crossbow builds: ursacomputing/crossbow @ actions-952

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-11-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

pitrou · 2021-10-14T10:11:47Z

Also, can you rebase from master to ensure that we get the latest CI fixes?

michalursa · 2021-10-14T23:47:44Z

@pitrou I rebased. It is ready for review.

pitrou · 2021-10-15T10:11:03Z

@github-actions crossbow submit -g cpp

github-actions · 2021-10-15T10:11:59Z

Revision: 9d5bceed669496e863bfde9e59c22d4fcaf3ea25

Submitted crossbow builds: ursacomputing/crossbow @ actions-966

Task	Status
test-conda-cpp
test-conda-cpp-valgrind
test-debian-11-cpp
test-fedora-33-cpp
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

cpp/src/arrow/compute/exec/hash_join.cc

pitrou · 2021-10-15T10:23:02Z

@github-actions crossbow submit -g r

pitrou · 2021-10-15T10:23:24Z

@bkietz Are you available to give a quick review on this? If CI is ok, it would be nice to have this fix in 6.0.0.

github-actions · 2021-10-15T10:24:01Z

Revision: 9d5bceed669496e863bfde9e59c22d4fcaf3ea25

Submitted crossbow builds: ursacomputing/crossbow @ actions-967

Task	Status
conda-linux-gcc-py36-cpu-r40
conda-linux-gcc-py37-cpu-r41
conda-osx-clang-py36-r40
conda-osx-clang-py37-r41
conda-win-vs2017-py36-r40
conda-win-vs2017-py37-r41
homebrew-r-autobrew
test-r-arrow-backwards-compatibility
test-r-depsource-auto
test-r-depsource-system
test-r-devdocs
test-r-gcc-11
test-r-install-local
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-ubuntu-gcc-release-latest
test-r-rocker-r-base-latest
test-r-rstudio-r-base-3.6-bionic
test-r-rstudio-r-base-3.6-centos7-devtoolset-8
test-r-rstudio-r-base-3.6-centos8
test-r-rstudio-r-base-3.6-opensuse15
test-r-rstudio-r-base-3.6-opensuse42
test-r-ubuntu-21.04
test-r-versions
test-ubuntu-18.04-r-sanitizer

bkietz

Just one nit, LGTM

cpp/src/arrow/compute/exec/hash_join.cc

kou · 2021-10-18T21:05:41Z

@github-actions crossbow submit -g cpp -g r

github-actions · 2021-10-18T21:06:46Z

Revision: 9fd1c024392207bc3622da6a3307c06272efbc4c

Submitted crossbow builds: ursacomputing/crossbow @ actions-994

Task	Status
conda-linux-gcc-py36-cpu-r40
conda-linux-gcc-py37-cpu-r41
conda-osx-clang-py36-r40
conda-osx-clang-py37-r41
conda-win-vs2017-py36-r40
conda-win-vs2017-py37-r41
homebrew-r-autobrew
test-conda-cpp
test-conda-cpp-valgrind
test-debian-11-cpp
test-fedora-33-cpp
test-r-arrow-backwards-compatibility
test-r-depsource-auto
test-r-depsource-system
test-r-devdocs
test-r-gcc-11
test-r-install-local
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-ubuntu-gcc-release-latest
test-r-rocker-r-base-latest
test-r-rstudio-r-base-3.6-bionic
test-r-rstudio-r-base-3.6-centos7-devtoolset-8
test-r-rstudio-r-base-3.6-centos8
test-r-rstudio-r-base-3.6-opensuse15
test-r-rstudio-r-base-3.6-opensuse42
test-r-ubuntu-21.04
test-r-versions
test-ubuntu-18.04-cpp
test-ubuntu-18.04-cpp-release
test-ubuntu-18.04-cpp-static
test-ubuntu-18.04-r-sanitizer
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-14
test-ubuntu-20.04-cpp-17
test-ubuntu-20.04-cpp-thread-sanitizer

kou · 2021-10-19T00:27:17Z

@michalursa Can we merge this?

michalursa · 2021-10-19T04:09:31Z

@michalursa Can we merge this?

@kou Yes, I believe so. This is a relatively simple change (more precisely 3 simple fixes put together) and I don't see any errors that could be related.

kou · 2021-10-19T04:46:33Z

OK.
I merge this.

ursabot · 2021-10-19T04:48:17Z

Benchmark runs are scheduled for baseline = 6e1293b and contender = d9ef519. d9ef519 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.51% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.36% ⬆️0.09%] ursa-thinkcentre-m75q
Supported benchmarks:
ursa-i9-9960x: langs = Python, R, JavaScript
ursa-thinkcentre-m75q: langs = C++, Java
ec2-t3-xlarge-us-east-2: cloud = True

#11350 moved ThreadIndexer from `aggregate_node.cc` to `compute/exec/util.h` to allow usage from the join node and incremented its capacity, but did not remove the original implementation. Closes #11463 from bkietz/remove-duplicated-ThreadIndexer Authored-by: Benjamin Kietzman <bengilgit@gmail.com> Signed-off-by: David Li <li.davidm96@gmail.com>

github-actions bot added the Component: C++ label Oct 7, 2021

michalursa force-pushed the ARROW-14211-hash-join-tsan branch from 16ded42 to 9682388 Compare October 7, 2021 04:47

pitrou requested a review from bkietz October 7, 2021 07:42

pitrou reviewed Oct 7, 2021

View reviewed changes

cpp/src/arrow/compute/exec/tpch_test.cc Outdated Show resolved Hide resolved

pitrou reviewed Oct 7, 2021

View reviewed changes

cpp/src/arrow/compute/exec/tpch_test.cc Outdated Show resolved Hide resolved

lidavidm mentioned this pull request Oct 7, 2021

ARROW-14247: [C++] Fix Valgrind errors in parquet-arrow-test #11356

Closed

michalursa changed the title ~~ARROW-14197: [C++][Compute] Fixing thread sanitizer problems in hash join node~~ ARROW-14211: [C++][Compute] Fixing thread sanitizer problems in hash join node Oct 7, 2021

michalursa force-pushed the ARROW-14211-hash-join-tsan branch from 9682388 to 7c80dbd Compare October 7, 2021 19:17

michalursa force-pushed the ARROW-14211-hash-join-tsan branch from 679309b to 3c32de9 Compare October 11, 2021 10:06

michalursa force-pushed the ARROW-14211-hash-join-tsan branch from 3c32de9 to f1dae79 Compare October 11, 2021 21:44

michalursa force-pushed the ARROW-14211-hash-join-tsan branch from f1dae79 to 1deb369 Compare October 11, 2021 22:47

michalursa force-pushed the ARROW-14211-hash-join-tsan branch from 1deb369 to 3d1ce0c Compare October 12, 2021 06:43

michalursa force-pushed the ARROW-14211-hash-join-tsan branch 2 times, most recently from 37993cf to c94bd41 Compare October 12, 2021 22:39

michalursa force-pushed the ARROW-14211-hash-join-tsan branch from c94bd41 to 9d5bcee Compare October 14, 2021 23:11

pitrou reviewed Oct 15, 2021

View reviewed changes

cpp/src/arrow/compute/exec/hash_join.cc Outdated Show resolved Hide resolved

michalursa force-pushed the ARROW-14211-hash-join-tsan branch from 9d5bcee to 08931a6 Compare October 16, 2021 02:48

michalursa mentioned this pull request Oct 18, 2021

ARROW-14181: [C++][Compute] Support for dictionaries in hash join #11446

Closed

bkietz approved these changes Oct 18, 2021

View reviewed changes

cpp/src/arrow/compute/exec/hash_join.cc Outdated Show resolved Hide resolved

michalursa force-pushed the ARROW-14211-hash-join-tsan branch from 08931a6 to 9fd1c02 Compare October 18, 2021 19:16

Fixing thread sanitizer problems in hash join node

720c4f5

michalursa force-pushed the ARROW-14211-hash-join-tsan branch from 9fd1c02 to 720c4f5 Compare October 18, 2021 21:39

kou closed this in d9ef519 Oct 19, 2021

bkietz mentioned this pull request Oct 19, 2021

ARROW-14382: [C++][Compute] Remove duplicated ThreadIndexer definition #11463

Closed

This was referenced Oct 13, 2021

[C++] Hashjoin + datasets hanging #29782

Closed

[C++] Valgrind and TSAN errors in arrow-compute-hash-join-node-test #29794

Closed

[C++][Compute] Remove duplicate ThreadIndexer definition #29950

Closed

ARROW-14211: [C++][Compute] Fixing thread sanitizer problems in hash join node #11350

ARROW-14211: [C++][Compute] Fixing thread sanitizer problems in hash join node #11350

Uh oh!

Conversation

michalursa commented Oct 7, 2021

Uh oh!

michalursa commented Oct 7, 2021

Uh oh!

github-actions bot commented Oct 7, 2021

Uh oh!

github-actions bot commented Oct 7, 2021

Uh oh!

michalursa commented Oct 7, 2021

Uh oh!

github-actions bot commented Oct 7, 2021

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 7, 2021

Uh oh!

michalursa commented Oct 11, 2021

Uh oh!

github-actions bot commented Oct 11, 2021

Uh oh!

michalursa commented Oct 11, 2021

Uh oh!

github-actions bot commented Oct 11, 2021

Uh oh!

michalursa commented Oct 11, 2021

Uh oh!

github-actions bot commented Oct 11, 2021

Uh oh!

michalursa commented Oct 11, 2021

Uh oh!

github-actions bot commented Oct 11, 2021

Uh oh!

michalursa commented Oct 12, 2021

Uh oh!

github-actions bot commented Oct 12, 2021

Uh oh!

pitrou commented Oct 14, 2021

Uh oh!

github-actions bot commented Oct 14, 2021

Uh oh!

pitrou commented Oct 14, 2021

Uh oh!

michalursa commented Oct 14, 2021

Uh oh!

pitrou commented Oct 15, 2021

Uh oh!

github-actions bot commented Oct 15, 2021

Uh oh!

Uh oh!

pitrou commented Oct 15, 2021

Uh oh!

pitrou commented Oct 15, 2021

Uh oh!

github-actions bot commented Oct 15, 2021

Uh oh!

bkietz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kou commented Oct 18, 2021

Uh oh!

github-actions bot commented Oct 18, 2021

Uh oh!

kou commented Oct 19, 2021

Uh oh!

michalursa commented Oct 19, 2021

Uh oh!

kou commented Oct 19, 2021

Uh oh!

ursabot commented Oct 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

ursabot commented Oct 19, 2021 •

edited

Loading