Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++] Upgrade mimalloc #27358

Closed
asfimport opened this issue Feb 2, 2021 · 12 comments
Closed

[C++] Upgrade mimalloc #27358

asfimport opened this issue Feb 2, 2021 · 12 comments

Comments

@asfimport
Copy link

I tried this in ARROW-11350 but ran into an issue (microsoft/mimalloc#353). That has since been resolved and we could apply a patch to bring it in. Or we can wait for it to get into a proper release.

There is also now a 1.7 release, which claims to work on the Apple M1, as well as a 2.0 version, which claims better performance.

Reporter: Neal Richardson / @nealrichardson
Assignee: Antoine Pitrou / @pitrou

PRs and other links:

Note: This issue was originally created as ARROW-11475. Please see the migration documentation for further details.

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
It may be interesting to also exercise ARROW-11433 with those new mimalloc versions. cc @jonkeane

@asfimport
Copy link
Author

Jonathan Keane / @jonkeane:
Absolutely — I'll watch this as it goes and exercise ARROW-11433 with whichever new version we go with. FWIW, I tested with version 1.6 and found good performance with that already.

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
@nealrichardson @jonkeane did you have a chance to run benchmarks (especially on macOS)?

@asfimport
Copy link
Author

Krisztian Szucs / @kszucs:
Issue resolved by pull request 9894
#9894

@asfimport
Copy link
Author

David Li / @lidavidm:
Reverted in #10024 as we found performance regressions and a bug in mimalloc (microsoft/mimalloc#363). 

@asfimport
Copy link
Author

Neal Richardson / @nealrichardson:
There were new releases of mimalloc last week:

Latest release tag: v2.0.2 (beta, 2021-06-17).
Latest stable tag: v1.7.2 (2021-06-17).

The issue #363 that David referenced appears to have been worked around at least. IDK about performance but we could try again.

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
Should this be tried again?

@asfimport
Copy link
Author

@asfimport
Copy link
Author

Neal Richardson / @nealrichardson:
Sure, why not? Not sure if the issues have been addressed upstream but only one way to find out.

@asfimport
Copy link
Author

Antoine Pitrou / @pitrou:
At least the 1.7.x release would be good to test. Even if the issues were not specifically addressed, it would bring us to a more recent release.

@asfimport
Copy link
Author

Kouhei Sutou / @kou:
Issue resolved by pull request 12020
#12020

@asfimport
Copy link
Author

Jeroen / @jeroen:
FYI, we had to revert back to mimalloc 1.7.2 to build on MacOS 10.13 high-sierra, because 1.7.3 failed to build with xcode 10.1:

 

[ 60%] Building C object CMakeFiles/mimalloc-static.dir/src/alloc.c.o
/usr/local/Homebrew/Library/Homebrew/shims/mac/super/clang -DMI_STATIC_LIB -I/tmp/apache-arrow-20220126-14221-1qo4vm7/apache-arrow-7.0.0/build/mimalloc_ep-prefix/src/mimalloc_ep/include -Qunused-arguments -O3 -DNDEBUG -DNDEBUG -fPIC   -Qunused-arguments -O3 -DNDEBUG -DNDEBUG -fPIC -fPIC -Wall -Wextra -Wno-unknown-pragmas -fvisibility=hidden -Wstrict-prototypes -Wpedantic -Wno-static-in-inline -ftls-model=local-dynamic -MD -MT CMakeFiles/mimalloc-static.dir/src/alloc.c.o -MF CMakeFiles/mimalloc-static.dir/src/alloc.c.o.d -o CMakeFiles/mimalloc-static.dir/src/alloc.c.o -c /tmp/apache-arrow-20220126-14221-1qo4vm7/apache-arrow-7.0.0/build/mimalloc_ep-prefix/src/mimalloc_ep/src/alloc.c
/tmp/apache-arrow-20220126-14221-1qo4vm7/apache-arrow-7.0.0/build/mimalloc_ep-prefix/src/mimalloc_ep/src/alloc.c:489:24: error: address argument to atomic operation must be a pointer to non-const _Atomic type ('const _Atomic(mi_threadid_t) *' invalid)
  if (mi_likely(tid == mi_atomic_load_relaxed(&segment->thread_id) && page->flags.full_aligned == 0)) {  // the thread id matches and it is not a full page, nor has aligned blocks
                       ^                      ~~~~~~~~~~~~~~~~~~~
/tmp/apache-arrow-20220126-14221-1qo4vm7/apache-arrow-7.0.0/build/mimalloc_ep-prefix/src/mimalloc_ep/include/mimalloc-atomic.h:47:50: note: expanded from macro 'mi_atomic_load_relaxed'
#define mi_atomic_load_relaxed(p)                mi_atomic(load_explicit)(p,mi_memory_order(relaxed))
                                                 ^                        ~
/tmp/apache-arrow-20220126-14221-1qo4vm7/apache-arrow-7.0.0/build/mimalloc_ep-prefix/src/mimalloc_ep/include/mimalloc-atomic.h:35:33: note: expanded from macro 'mi_atomic'
#define  mi_atomic(name)        atomic_##name
                                ^
<scratch space>:470:1: note: expanded from here
atomic_load_explicit
^
/Applications/Xcode-10.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/10.0.0/include/stdatomic.h:135:30: note: expanded from macro 'atomic_load_explicit'
#define atomic_load_explicit __c11_atomic_load
                             ^
/tmp/apache-arrow-20220126-14221-1qo4vm7/apache-arrow-7.0.0/build/mimalloc_ep-prefix/src/mimalloc_ep/include/mimalloc-internal.h:153:46: note: expanded from macro 'mi_likely'
#define mi_likely(x)       __builtin_expect((x),1)
                                             ^
1 error generated.
make[5]: *** [CMakeFiles/mimalloc-static.dir/src/alloc.c.o] Error 1
make[4]: *** [CMakeFiles/mimalloc-static.dir/all] Error 2
make[3]: *** [all] Error 2
make[2]: *** [mimalloc_ep-prefix/src/mimalloc_ep-stamp/mimalloc_ep-build] Error 2
make[1]: *** [CMakeFiles/mimalloc_ep.dir/all] Error 2
 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants