You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pitrou (via GitHub)" <gi...@apache.org> on 2023/07/04 16:35:43 UTC
[GitHub] [arrow] pitrou opened a new pull request, #36461: GH-36327: [C++][CI] Fix Valgrind failures
pitrou opened a new pull request, #36461:
URL: https://github.com/apache/arrow/pull/36461
* Silence Valgrind error in OpenSSL
* Make sure the entire data buffer of pairwise_diff is initialized
* Make sure the bitmaps of REE-encoded data are initialized
--
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 diff in pull request #36461: GH-36327: [C++][CI] Fix Valgrind failures
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #36461:
URL: https://github.com/apache/arrow/pull/36461#discussion_r1252269362
##########
cpp/src/arrow/compute/kernels/ree_util_internal.cc:
##########
@@ -62,7 +63,7 @@ Result<std::shared_ptr<ArrayData>> PreallocateValuesArray(
std::vector<std::shared_ptr<Buffer>> values_data_buffers;
std::shared_ptr<Buffer> validity_buffer = NULLPTR;
if (has_validity_buffer) {
- ARROW_ASSIGN_OR_RAISE(validity_buffer, AllocateBitmap(length, pool));
+ ARROW_ASSIGN_OR_RAISE(validity_buffer, AllocateEmptyBitmap(length, pool));
Review Comment:
We do this in other places already. Initializing a buffer that's just been allocated (and that will be accessed just after) should be reasonably cheap.
I would welcome another solution, but we don't want to disable the Valgrid checks as they're important.
##########
cpp/src/arrow/compute/kernels/ree_util_internal.cc:
##########
@@ -62,7 +63,7 @@ Result<std::shared_ptr<ArrayData>> PreallocateValuesArray(
std::vector<std::shared_ptr<Buffer>> values_data_buffers;
std::shared_ptr<Buffer> validity_buffer = NULLPTR;
if (has_validity_buffer) {
- ARROW_ASSIGN_OR_RAISE(validity_buffer, AllocateBitmap(length, pool));
+ ARROW_ASSIGN_OR_RAISE(validity_buffer, AllocateEmptyBitmap(length, pool));
Review Comment:
We do this in other places already. Initializing a buffer that's just been allocated (and that will be accessed just after) should be reasonably cheap.
I would welcome another solution, but we don't want to disable the Valgrind checks as they're important.
--
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 #36461: GH-36327: [C++][CI] Fix Valgrind failures
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36461:
URL: https://github.com/apache/arrow/pull/36461#issuecomment-1620520675
@js8544 @felipecrv This touches some of the kernel code you've worked on.
--
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] felipecrv commented on a diff in pull request #36461: GH-36327: [C++][CI] Fix Valgrind failures
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #36461:
URL: https://github.com/apache/arrow/pull/36461#discussion_r1252265963
##########
cpp/src/arrow/compute/kernels/ree_util_internal.cc:
##########
@@ -62,7 +63,7 @@ Result<std::shared_ptr<ArrayData>> PreallocateValuesArray(
std::vector<std::shared_ptr<Buffer>> values_data_buffers;
std::shared_ptr<Buffer> validity_buffer = NULLPTR;
if (has_validity_buffer) {
- ARROW_ASSIGN_OR_RAISE(validity_buffer, AllocateBitmap(length, pool));
+ ARROW_ASSIGN_OR_RAISE(validity_buffer, AllocateEmptyBitmap(length, pool));
Review Comment:
Do we have to pay the cost of initializing the buffers even though every value will be overwritten by the code that uses this buffer?
--
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] conbench-apache-arrow[bot] commented on pull request #36461: GH-36327: [C++][CI] Fix Valgrind failures
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #36461:
URL: https://github.com/apache/arrow/pull/36461#issuecomment-1627559758
Conbench analyzed the 6 benchmark runs on commit `42447e6a`.
There were 5 benchmark results indicating a performance regression:
- Commit Run on `ursa-thinkcentre-m75q` at [2023-07-06 13:08:58Z](http://conbench.ursa.dev/compare/runs/a10994400bd845cfa87671cda1f2c480...2e165be1b1fa42ea9a36763d8e3ed8c4/)
- [params=<STATIC_VECTOR(std::shared_ptr<int>)>, source=cpp-micro, suite=arrow-small-vector-benchmark](http://conbench.ursa.dev/compare/benchmarks/064a694298197ede800069c36e7a1ec4...064a6bd283b27c6c80002cdf02ab9f07)
- [params=DecodeArrow_Dense/4096, source=cpp-micro, suite=parquet-encoding-benchmark](http://conbench.ursa.dev/compare/benchmarks/064a694540c97e718000a8fd2fb5ba6a...064a6bd518c77b8b8000518057f6292c)
- and 3 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/14884578148) has more details.
--
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 merged pull request #36461: GH-36327: [C++][CI] Fix Valgrind failures
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #36461:
URL: https://github.com/apache/arrow/pull/36461
--
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 #36461: GH-36327: [C++][CI] Fix Valgrind failures
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36461:
URL: https://github.com/apache/arrow/pull/36461#issuecomment-1620522912
Revision: 50c2074a7c71925f0bd789ac270dbf08af8fb832
Submitted crossbow builds: [ursacomputing/crossbow @ actions-68f5854ef3](https://github.com/ursacomputing/crossbow/branches/all?query=actions-68f5854ef3)
|Task|Status|
|----|------|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5456886554/jobs/9930286340)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/5456887589/jobs/9930288696)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5456888597/jobs/9930290865)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/runs/14773323188)|
|test-cuda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5456886747/jobs/9930286777)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/5456887967/jobs/9930289485)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/5456887183/jobs/9930287938)|
|test-fedora-35-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5456887091/jobs/9930287625)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5456887357/jobs/9930288312)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/5456888405/jobs/9930290426)|
|test-ubuntu-20.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/5456886885/jobs/9930287061)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/5456888866/jobs/9930291346)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/5456888231/jobs/9930289995)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/5456886628/jobs/9930286479)|
--
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 #36461: GH-36327: [C++][CI] Fix Valgrind failures
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #36461:
URL: https://github.com/apache/arrow/pull/36461#issuecomment-1620520411
@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