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

Update to latest jemallocator and jemalloc-sys #20848

Closed
wants to merge 6 commits into from

Conversation

@gnzlbg
Copy link

gnzlbg commented May 22, 2018

Updates the allocator component to use the latest jemallocator.

cc @SimonSapin

  • ./mach build -d does not report any errors

  • ./mach test-tidy does not report any errors

  • These changes fix #20645

  • There are tests for these changes: everything that allocates memory tests them.


This change is Reviewable

@highfive
Copy link

highfive commented May 22, 2018

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @glennw (or someone else) soon.

@SimonSapin
Copy link
Member

SimonSapin commented May 22, 2018

Thanks for the PR! What changed so that you expect jemalloc 5 to now build with Servo’s old Android NDK?

It looks like this doesn’t build right now. First because of error[E0658]: use of unstable library feature 'nonnull_cast' in jemallocator because we’re stuck on an old Nightly at the moment. Nothing to do with this PR, this is tracked at #20844. Next I guess use std::os::raw::{void}; will error because it should be c_void. (By the way we prefer not using {} braces for a single import.)

Other than this the diff looks good.

@gnzlbg
Copy link
Author

gnzlbg commented May 22, 2018

What changed so that you expect jemalloc 5 to now build with Servo’s old Android NDK?

jemallocator now exposes the bg_thread feature enabled by default. I hope that by disabling it for the old Android NDK it will build. But I am not sure if this will work, so I wanted to just try it out.

@gnzlbg
Copy link
Author

gnzlbg commented May 23, 2018

@SimonSapin I don't know what went wrong with the second travis build bot :/

@gnzlbg
Copy link
Author

gnzlbg commented May 23, 2018

I also don't know what's wrong with the TaskCluster bot :/

@SimonSapin
Copy link
Member

SimonSapin commented May 23, 2018

Feel free to ignore TaskCluster, we’re not gating on it yet.

Travis failed on nonnull_cast. As I mentioned above, this is unrelated to this PR and blocked on #20844.

@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2018

The latest upstream changes (presumably #20882) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin SimonSapin force-pushed the gnzlbg:jemallocator branch from 7c514e8 to fc11018 May 30, 2018
@SimonSapin
Copy link
Member

SimonSapin commented May 30, 2018

Rebased now that we upgraded the compiler. Let’s see if this passes CI.

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2018

📌 Commit fc11018 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2018

Testing commit fc11018 with merge 5defaf6...

bors-servo added a commit that referenced this pull request May 30, 2018
Update to latest jemallocator and jemalloc-sys

Updates the allocator component to use the latest jemallocator.

cc @SimonSapin

- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes fix #20645

- [x] There are tests for these changes: everything that allocates memory tests them.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20848)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2018

💔 Test failed - linux-rel-wpt

@SimonSapin SimonSapin force-pushed the gnzlbg:jemallocator branch from fc11018 to 30d1090 May 30, 2018
@SimonSapin
Copy link
Member

SimonSapin commented May 30, 2018

@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2018

📌 Commit 30d1090 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2018

Testing commit 30d1090 with merge 2a4dbd6...

bors-servo added a commit that referenced this pull request May 30, 2018
Update to latest jemallocator and jemalloc-sys

Updates the allocator component to use the latest jemallocator.

cc @SimonSapin

- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes fix #20645

- [x] There are tests for these changes: everything that allocates memory tests them.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20848)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2018

💔 Test failed - android

@gnzlbg
Copy link
Author

gnzlbg commented May 30, 2018

Nice, it failed! @SimonSapin is that the same error that you were previously getting?

I am going to disable the background thread for all builds and see if then the build succeeds. Do not merge anything if the test pass; if they do I will try to more fine-grainedly disable it only for that particular Android target.

--- stderr
configure: WARNING: using cross tools not prefixed with host triplet
In file included from /home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/include/jemalloc/internal/jemalloc_internal_includes.h:78:0,
                 from /home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/src/jemalloc.c:3:
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/include/jemalloc/internal/jemalloc_internal_inlines_a.h: In function 'malloc_getcpu':
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/include/jemalloc/internal/jemalloc_internal_inlines_a.h:14:2: warning: implicit declaration of function 'sched_getcpu' [-Wimplicit-function-declaration]
  return (malloc_cpuid_t)sched_getcpu();
  ^
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/src/jemalloc.c: In function 'malloc_init_hard_recursible':
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/src/jemalloc.c:1355:2: warning: implicit declaration of function 'pthread_atfork' [-Wimplicit-function-declaration]
  if (pthread_atfork(jemalloc_prefork, jemalloc_postfork_parent,
  ^
In file included from /home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/include/jemalloc/internal/jemalloc_internal_includes.h:78:0,
                 from /home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/src/base.c:3:
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/include/jemalloc/internal/jemalloc_internal_inlines_a.h: In function 'malloc_getcpu':
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/include/jemalloc/internal/jemalloc_internal_inlines_a.h:14:2: warning: implicit declaration of function 'sched_getcpu' [-Wimplicit-function-declaration]
  return (malloc_cpuid_t)sched_getcpu();
  ^
In file included from /home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/include/jemalloc/internal/jemalloc_internal_includes.h:78:0,
                 from /home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/src/arena.c:3:
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/include/jemalloc/internal/jemalloc_internal_inlines_a.h: In function 'malloc_getcpu':
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/include/jemalloc/internal/jemalloc_internal_inlines_a.h:14:2: warning: implicit declaration of function 'sched_getcpu' [-Wimplicit-function-declaration]
  return (malloc_cpuid_t)sched_getcpu();
  ^
In file included from /home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/include/jemalloc/internal/jemalloc_internal_includes.h:78:0,
                 from /home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/src/background_thread.c:3:
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/include/jemalloc/internal/jemalloc_internal_inlines_a.h: In function 'malloc_getcpu':
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/include/jemalloc/internal/jemalloc_internal_inlines_a.h:14:2: warning: implicit declaration of function 'sched_getcpu' [-Wimplicit-function-declaration]
  return (malloc_cpuid_t)sched_getcpu();
  ^
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/src/background_thread.c: In function 'set_current_thread_affinity':
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/src/background_thread.c:86:2: error: unknown type name 'cpu_set_t'
  cpu_set_t cpuset;
  ^
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/src/background_thread.c:87:2: warning: implicit declaration of function 'CPU_ZERO' [-Wimplicit-function-declaration]
  CPU_ZERO(&cpuset);
  ^
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/src/background_thread.c:88:2: warning: implicit declaration of function 'CPU_SET' [-Wimplicit-function-declaration]
  CPU_SET(cpu, &cpuset);
  ^
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/src/background_thread.c:89:2: warning: implicit declaration of function 'sched_setaffinity' [-Wimplicit-function-declaration]
  int ret = sched_setaffinity(0, sizeof(cpu_set_t), &cpuset);
  ^
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/src/background_thread.c:89:40: error: 'cpu_set_t' undeclared (first use in this function)
  int ret = sched_setaffinity(0, sizeof(cpu_set_t), &cpuset);
                                        ^
/home/servo/buildbot/slave/android/build/target/armv7-linux-androideabi/debug/build/jemalloc-sys-3d97431a4930f227/out/jemalloc/src/background_thread.c:89:40: note: each undeclared identifier is reported only once for each function it appears in
make: *** [src/background_thread.sym.o] Error 1
make: *** Waiting for unfinished jobs....
thread 'main' panicked at 'command did not execute successfully: "make" "-j" "4"
expected success, got: exit code: 2', /home/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/jemalloc-sys-0.1.6/build.rs:257:9
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:211
   3: std::panicking::default_hook
             at libstd/panicking.rs:227
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:463
   5: std::panicking::begin_panic_fmt
             at libstd/panicking.rs:350
   6: build_script_build::run
             at ./build.rs:257
   7: build_script_build::main
             at ./build.rs:202
   8: std::rt::lang_start::{{closure}}
             at /checkout/src/libstd/rt.rs:74
   9: std::panicking::try::do_call
             at libstd/rt.rs:59
             at libstd/panicking.rs:310
  10: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:105
  11: std::rt::lang_start_internal
             at libstd/panicking.rs:289
             at libstd/panic.rs:374
             at libstd/rt.rs:58
  12: std::rt::lang_start
             at /checkout/src/libstd/rt.rs:74
  13: main
  14: __libc_start_main
  15: <unknown>
@gnzlbg gnzlbg force-pushed the gnzlbg:jemallocator branch from 30d1090 to 023eebd May 30, 2018
gnzlbg added 2 commits May 30, 2018
@SimonSapin
Copy link
Member

SimonSapin commented May 30, 2018

error: unknown type name 'cpu_set_t', yes, this looks the same.

@bors-servo try

@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2018

Trying commit 68f6750 with merge fd5e632...

bors-servo added a commit that referenced this pull request May 30, 2018
Update to latest jemallocator and jemalloc-sys

Updates the allocator component to use the latest jemallocator.

cc @SimonSapin

- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes fix #20645

- [x] There are tests for these changes: everything that allocates memory tests them.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20848)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented May 30, 2018

💔 Test failed - android

@gnzlbg
Copy link
Author

gnzlbg commented May 30, 2018

Thanks! I've added the new info to the bug report upstream. I am closing this since... there is nothing we can do until the next jemalloc release. jemalloc/jemalloc#1175 (comment)

If a patch lands I might update jemallocator upstream to the appropriate master commit of jemalloc upstream, and re-open this to try again. Maybe some day this will work again :/

@gnzlbg gnzlbg closed this May 30, 2018
@SimonSapin
Copy link
Member

SimonSapin commented May 30, 2018

Thanks for your work on this. I think that upgrading the NDK to a more recent version would also fix this, but that comes with its own set of unrelated issues. I’ll reopen this PR if and when we do that.

@gnzlbg
Copy link
Author

gnzlbg commented May 30, 2018

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

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.