You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "felipecrv (via GitHub)" <gi...@apache.org> on 2023/02/14 23:50:48 UTC
[GitHub] [arrow] felipecrv opened a new pull request, #34185: MINOR: [C++] Remove std:: before malloc and free to let them be jemalloc versions
felipecrv opened a new pull request, #34185:
URL: https://github.com/apache/arrow/pull/34185
Since `jemalloc.h` does
# define malloc je_malloc
when JEMALLOC_MANGLE is defined, I'm getting this error in CI
/arrow/cpp/src/arrow/vendored/ProducerConsumerQueue.h: In constructor 'arrow_vendored::folly::ProducerConsumerQueue<T>::ProducerConsumerQueue(uint32_t)':
/arrow/cpp/src/arrow/vendored/ProducerConsumerQueue.h:82:39: error: 'je_arrow_malloc' is not a member of 'std'; did you mean 'je_arrow_malloc'?
82 | records_(static_cast<T*>(std::malloc(sizeof(T) * size))),
| ^~~~~~
jemalloc_ep-prefix/src/jemalloc_ep/dist/include/jemalloc/jemalloc.h:254:32: note: 'je_arrow_malloc' declared here
254 | void JEMALLOC_SYS_NOTHROW *je_malloc(size_t size)
| ^~~~~~~~~
/arrow/cpp/src/arrow/vendored/ProducerConsumerQueue.h: In destructor 'arrow_vendored::folly::ProducerConsumerQueue<T>::~ProducerConsumerQueue()':
/arrow/cpp/src/arrow/vendored/ProducerConsumerQueue.h:106:10: error: 'je_arrow_free' is not a member of 'std'; did you mean 'je_arrow_free'?
106 | std::free(records_);
| ^~~~
jemalloc_ep-prefix/src/jemalloc_ep/dist/include/jemalloc/jemalloc.h:269:43: note: 'je_arrow_free' declared here
269 | JEMALLOC_EXPORT void JEMALLOC_SYS_NOTHROW je_free(void *ptr)
| ^~~~~~~
Removing the `std::` prefix fixes this.
--
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] lidavidm commented on pull request #34185: MINOR: [C++] Remove std:: before malloc and free to let them be jemalloc versions
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431397970
Ah...you could `#ifdef ... #undef` (I guess you'd have to redefine it though), or possibly, we should consider whether we want jemalloc to `#define malloc` in the first place (I don't think that's what we intended)
--
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] assignUser commented on pull request #34185: MINOR: [C++] Remove std:: before malloc and free to let them be jemalloc versions
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1430576567
Mhm this is out of my wheelhouse so I'll defer to someone else :D
--
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 #34185: GH-34206: [C++] Don't let jemalloc defines affect unity builds
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431762077
* Closes: #34206
--
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 pull request #34185: MINOR: [C++] Don't let jemalloc defines affect unity builds
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431757530
@lidavidm @zeroshade please take a look now.
After this is merged I will rebase my REE PR and hopefully get it to pass CI.
--
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] lidavidm commented on pull request #34185: MINOR: [C++] Remove std:: before malloc and free to let them be jemalloc versions
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431310827
Could also figure out how/why the jemalloc header is getting included there in the first place - I don't think we intended jemalloc to replace libc malloc everywhere
--
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] kou commented on pull request #34185: GH-34206: [C++] Don't let jemalloc defines affect unity builds
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431931617
I think that we should not mix `memory_pool_jemalloc.cc` to other sources instead of `#undef`:
```diff
diff --git a/cpp/src/arrow/CMakeLists.txt b/cpp/src/arrow/CMakeLists.txt
index 6382fdfb3a..33cf3d396d 100644
--- a/cpp/src/arrow/CMakeLists.txt
+++ b/cpp/src/arrow/CMakeLists.txt
@@ -243,6 +243,8 @@ set(ARROW_SRCS
if(ARROW_JEMALLOC)
list(APPEND ARROW_SRCS memory_pool_jemalloc.cc)
+ set_source_files_properties(memory_pool_jemalloc.cc
+ PROPERTIES SKIP_UNITY_BUILD_INCLUSION ON)
endif()
append_avx2_src(util/bpacking_avx2.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] felipecrv commented on pull request #34185: MINOR: [C++] Remove std:: before malloc and free to let them be jemalloc versions
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431589952
@lidavidm thank you for clarifying the intentions of the jemalloc integration. I didn't have enough context about this. I will wait for the current round of builds to finish and send an alternative fix that doesn't remove the `std::`
--
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] kou merged pull request #34185: GH-34206: [C++] Don't let jemalloc defines affect unity builds
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #34185:
URL: https://github.com/apache/arrow/pull/34185
--
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 pull request #34185: GH-34206: [C++] Don't let jemalloc defines affect unity builds
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431969764
@kou yeah, that makes total sense. Done.
--
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 pull request #34185: GH-34206: [C++] Don't let jemalloc defines affect unity builds
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431999178
@kou the issue manifested in https://github.com/apache/arrow/pull/33641 which is currently going through the CI build with this PR's commit on top. A true confirmation is a green build there.
--
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 pull request #34185: MINOR: [C++] Remove std:: before malloc and free to let them be jemalloc versions
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431356124
> Could also figure out how/why the jemalloc header is getting included there in the first place - I don't think we intended jemalloc to replace libc malloc everywhere
Is it even possible to not include that header in the compilation unit of a unity build?
--
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] kou commented on pull request #34185: GH-34206: [C++] Don't let jemalloc defines affect unity builds
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431996374
Did you confirm that the change fix the problem on your environment?
--
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] lidavidm commented on pull request #34185: MINOR: [C++] Remove std:: before malloc and free to let them be jemalloc versions
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431558735
We do want the mangle so that our copy of jemalloc does not conflict with any copies of jemalloc used by applications - so it seems unfortunate that mangling _also_ makes jemalloc try to redefine malloc. Possibly we can insert an `#undef malloc` after including jemalloc.
--
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 pull request #34185: MINOR: [C++] Remove std:: before malloc and free to let them be jemalloc versions
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431304617
I will extract the second commit as a separate PR if it fixes the MSVC linker errors.
--
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] ursabot commented on pull request #34185: GH-34206: [C++] Don't let jemalloc defines affect unity builds
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1433283738
['Python', 'R'] benchmarks have high level of regressions.
[test-mac-arm](https://conbench.ursa.dev/compare/runs/bd095c06f12842e2afcd08afb6440e90...783c012e5ddb4f3a8c2fa4eed605f0d2/)
--
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 pull request #34185: MINOR: [C++] Remove std:: before malloc and free to let them be jemalloc versions
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1430541996
@zeroshade @assignUser
--
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] zeroshade commented on pull request #34185: MINOR: [C++] Remove std:: before malloc and free to let them be jemalloc versions
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431586261
I second @lidavidm's suggestion of inserting an `#undef malloc` after including jemalloc
--
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 pull request #34185: MINOR: [C++] Remove std:: before malloc and free to let them be jemalloc versions
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431543176
Thank you for the suggestion: I will create an issue.
I think the defining of malloc and free can be disabled if we don't define `JEMALLOC_MANGLE` like it's done here
https://github.com/apache/arrow/blob/798b417ec342051f3ff30b9953dcb1f1b71dc9e8/cpp/src/arrow/memory_pool_jemalloc.cc#L28-L34
--
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] ursabot commented on pull request #34185: GH-34206: [C++] Don't let jemalloc defines affect unity builds
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1433281428
Benchmark runs are scheduled for baseline = 4550c9b35e6774301f65a5678cfbb9753ceca614 and contender = b40fb2c7980bff96c1a4be653309731131d36ed4. b40fb2c7980bff96c1a4be653309731131d36ed4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/1bb1deccc281417b996b29a97c54a0df...3d30b9b7316641bdb3b76bdfe1b443b2/)
[Failed :arrow_down:0.09% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/bd095c06f12842e2afcd08afb6440e90...783c012e5ddb4f3a8c2fa4eed605f0d2/)
[Finished :arrow_down:0.26% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/9d59eab11cf34ebdbc0cd3891b6c990e...ff1a9b8d6a6a4404a373a82695821c01/)
[Finished :arrow_down:0.29% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/3bbd7d9c996e48d9ab15874eb8ed2a06...cbfae60bba6e4839a7ab9981394929fb/)
Buildkite builds:
[Finished] [`b40fb2c7` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2384)
[Failed] [`b40fb2c7` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2414)
[Finished] [`b40fb2c7` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2382)
[Finished] [`b40fb2c7` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2406)
[Finished] [`4550c9b3` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2383)
[Finished] [`4550c9b3` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2413)
[Finished] [`4550c9b3` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2381)
[Finished] [`4550c9b3` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2405)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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] lidavidm commented on pull request #34185: MINOR: [C++] Remove std:: before malloc and free to let them be jemalloc versions
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #34185:
URL: https://github.com/apache/arrow/pull/34185#issuecomment-1431398737
If this unsticks the build though maybe let's go with this and file an issue for followup (on what we want to do with jemalloc)
--
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 #34185: MINOR: [C++] Remove std:: before malloc and free to let them be jemalloc versions
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #34185:
URL: https://github.com/apache/arrow/pull/34185#discussion_r1107278915
##########
cpp/cmake_modules/ThirdpartyToolchain.cmake:
##########
@@ -2292,6 +2292,7 @@ macro(build_benchmark)
file(MAKE_DIRECTORY "${GBENCHMARK_INCLUDE_DIR}")
add_library(benchmark::benchmark STATIC IMPORTED)
+ target_compile_definitions(benchmark::benchmark INTERFACE BENCHMARK_STATIC_DEFINE)
Review Comment:
My previous attempt at this change mistakenly deleted a crucial line. :sweat:
Fingers crossed that this will fix the MSVC issue.
--
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