Closed
Bug 1350581
Opened 7 years ago
Closed 6 years ago
Rust induced build error: Undefined symbols _je_malloc_usable_size referenced from heapsize::heap_size_of_impl in libgkrust.a
Categories
(Core :: Memory Allocator, enhancement)
Core
Memory Allocator
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox55 | --- | affected |
People
(Reporter: jwatt, Assigned: n.nethercote)
References
Details
(Whiteboard: [workaround: ac_add_options --disable-webrender])
On Mac debug builds run so slowly nowadays it's virtually impossible to get things done. I've come to the conclusion that I generally need to build with --disable-debug to be productive, but then I don't want optimizations to mess with the debugger. Hence I'm trying to build with both debug and optimization disabled. That is: ac_add_options --disable-debug --disable-optimize But I then get this error: Undefined symbols for architecture x86_64: "_je_malloc_usable_size", referenced from: heapsize::heap_size_of_impl::h6ba578567c9f2476 in libgkrust.a(gkrust-c86a490ee9df73f8.0.o) ld: symbol(s) not found for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation) I assume the reason our build machines don't encounter this is that they have debugging turned off when optimization is turned, and vice versa. Never both off together (we should add a machine to do that). From trying to follow the build config files it seemed like maybe I could get the symbol by using: ac_add_options --enable-replace-malloc --enable-jemalloc but that made no difference.
Comment 1•7 years ago
|
||
I'm hitting the same problem. I'm using --disable-optimize with non-debug builds for the same reasons as Jonathan.
Comment 2•7 years ago
|
||
The problem is with the heapsize crate, which assumes that a) the jemalloc malloc_usable_size symbol is available under a specific name that is an implementation detail of how jemalloc is hooked with rust and b) that jemalloc is used at all (we build rust code with jemalloc disabled). What's interesting, though, is that it only fails with optimizations disabled, which suggests the code is not actually used. So why use the crate if we're not actually using the code that makes it work?
Comment 3•7 years ago
|
||
Stylo disables the heapsize crate. WebRender should probably add a similar Cargo feature to do so as well.
Comment 4•7 years ago
|
||
(In reply to Simon Sapin (:SimonSapin) from comment #3) > Stylo disables the heapsize crate. WebRender should probably add a similar > Cargo feature to do so as well. Kats, can you look into doing this?
Flags: needinfo?(bugmail)
Comment 5•7 years ago
|
||
I can own this, sure.
Assignee: nobody → bugmail
Flags: needinfo?(bugmail)
See Also: → https://github.com/servo/webrender/issues/1016
Comment 6•7 years ago
|
||
So I quickly tried to hack this up, by removing the heapsize dependency from webrender_traits. But it looks like there are other crates that also have this dependency (e.g. app_units) so if we want to go this route we might have to propagate this feature all the way down to them. Simon, can you point me to where stylo is doing this? Maybe I'm misunderstanding.
Flags: needinfo?(simon.sapin)
Comment 7•7 years ago
|
||
> we build rust code with jemalloc disabled Is this really the case? I though Gecko’s jemalloc was used. Glandium, is there a function like malloc_usable_size that takes a pointer and returns a size? If so we can probably hack the heapsize crate to link to the appropriate symbol. njn, you were asking on IRC earlier how to make heapsize measure stylesheet objects. Do you want to use it with Gecko? If so perhaps we shouldn’t remove it :) If we do want to remove it, the idea is to make the dependency optional and enable it for Servo though Cargo’s "features" mechanism. Yes, this needs to be done in every crate that depends on heapsize. According to toolkit/library/rust/Cargo.lock, this is: * app_units * euclid * servo_geometry * style * webrender_traits The style crate used to do this, I’ve filed https://github.com/servo/servo/pull/16169 to do it again. In this case there are catch-all "servo" and "gecko" features. Regarding propagating all the way down, you’re correct that with a dep graph like A --> B --> C -(optional)-> heapsize, if A wants to enable heapsize in C then B also needs a feature to propagate this. However, all of the crates listed above also also depended on by at least one Servo-only crate, so heapsize can be enabled there. (When a crate has multiple dependents, Cargo compiles it with the union of all enabled features.)
Flags: needinfo?(simon.sapin) → needinfo?(n.nethercote)
Comment 8•7 years ago
|
||
(In reply to Simon Sapin (:SimonSapin) from comment #7) > > we build rust code with jemalloc disabled > > Is this really the case? I though Gecko’s jemalloc was used. Whatever gecko used is used (which may or may not be jemalloc)... by making rust code use the system malloc() instead of jemalloc. > Glandium, is there a function like malloc_usable_size that takes a pointer > and returns a size? If so we can probably hack the heapsize crate to link to > the appropriate symbol. moz_malloc_usable_size is the only symbol that I think we can use and that would work in all possible configurations of gecko.
Comment 9•7 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #8) > moz_malloc_usable_size is the only symbol that I think we can use and that > would work in all possible configurations of gecko. And even then, it needs to be weakly linked on some platforms... we might as well add a je_malloc_usable_size symbol in libxul to do the wrapping... That being said, that won't make the windows part of heapsize use the right thing either, since that apparently, for a reason I don't get, use Windows Heap APIs for that, which won't return anything useful for anything that was allocated in rust via jemalloc...
Comment 10•7 years ago
|
||
> moz_malloc_usable_size is the only symbol that I think we can use and that would work in all possible configurations of gecko.
What do you think of adding a Cargo feature to heapsize to make it link to moz_malloc_usable_size?
Getting the "usable size" of an allocation is fundamental to what this crate does, we need either a reliable way to get it or to disable the crate.
Assignee | ||
Comment 11•7 years ago
|
||
In Gecko, for various reasons, we pass a function (with type mozilla::MallocSizeOf) in to all the heap_size_of_children()-style functions. It's responsible for the actual measurement of heap blocks. I didn't do that when I implemented HeapSize in Servo because it wasn't necessary then, but it's necessary now for integration with DMD (https://developer.mozilla.org/en-US/docs/Mozilla/Performance/DMD) in Gecko. It would also fix this linking problem. I can do it early next week. Will I have to follow the complicated workflow in https://public.etherpad-mozilla.org/p/stylo ?
Flags: needinfo?(n.nethercote)
Comment 12•7 years ago
|
||
Changing the signature of methods is a breaking change, so it would have to be in a heapsize crate version number accordingly. The current version is 0.3.8, so 0.4.0. (Cargo extrapolates semantic versioning so that 0.a.b is considered compatible with 0.a.c, but 0.d.e is not.) Then every crate that uses heapsize will need to be updated. Even if there is no code change (perhaps because they use "derive") they need to opt into the incompatible version in Cargo.toml. This involves coordinating pull requests into multiple repositories, then finally in https://github.com/servo/servo. The Stylo workflow can be ignored for this. When the changes land in servo/servo, they’ll be automatically synchronized into integration/autoland. At that point, gecko will probably end up with two copies of heapsize (at different versions) in third-party/rust/ until WebRender is updated. That’s probably ok since neither is used at the moment.
Assignee | ||
Comment 13•7 years ago
|
||
> Then every crate that uses heapsize will need to be updated.
And there's something like a dozen of them, alas.
Updated•7 years ago
|
Whiteboard: [workaround: ac_add_options --disable-webrender]
Comment 14•7 years ago
|
||
Instead of doing this via a feature, would it be possible to somehow set an environment variable during the build, e.g. `HEAPSIZE_LINK_NAME=moz_malloc_usable_size cargo build` and have a build.rs script in heapsize that uses the environment variable to pick up the correct link name? I don't know if build.rs files are able to do something lke that, but would be simpler than having to propagate the feature all the way through.
Comment 15•7 years ago
|
||
In fact it looks like the existing build.rs does basically the same thing for the prefixed-jemalloc cfg attribute so this should totally be doable.
Comment 16•7 years ago
|
||
Here's a try push which does what I described above. I used [replace] to use a modified version of heapsize, but obviously I'd upstream that change. I verified that it fixes my OS X build locally (with --disable-optimize and --disable-debug). https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5f45997a6595aad12b1e7dc22749b98703eee50
Comment 17•7 years ago
|
||
I guess [replace] doesn't work on try. Here's one with the .cargo-checksum files hacked up: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d089156a2d508d83c1a872fd751143485da7345
Comment 18•7 years ago
|
||
That was a bad attempt. Here's a better one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f4e76c95df013a638535330c0570259fc43793a However I guess this might not address the issues that glandium pointed out in comment 9. I'm happy to submit a PR for this change if this is the way we want to go. njn, are you planning on implementing the other option you described in comment 11?
Flags: needinfo?(n.nethercote)
Assignee | ||
Comment 19•7 years ago
|
||
> njn, are you planning on implementing the other option you described in comment 11? I'm still working out exactly how best to do it. I had been thinking of leaving the heapsize crate as is, and introduce a similar trait (mallocsize?) that does what comment 11 suggests. (The two crates would be very similar.) That way I wouldn't have to update all those external crates used by Servo. But WebRender is used by both Servo and Gecko, right? So it would end up using both heapsize and mallocsize, which wouldn't solve this problem. Sounds like using cargo features to enable heapsize for all the relevant crates might be best, then.
Flags: needinfo?(n.nethercote)
Comment 20•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #19) > I'm still working out exactly how best to do it. I had been thinking of > leaving the heapsize crate as is, and introduce a similar trait > (mallocsize?) that does what comment 11 suggests. (The two crates would be > very similar.) That way I wouldn't have to update all those external crates > used by Servo. I'm not sure I follow this exactly. If you added a new trait, wouldn't you want to update all the crates that currently use heapsize to use your new trait as well? Or would you only do the crates that gecko includes? > But WebRender is used by both Servo and Gecko, right? Yes. > So it would end up > using both heapsize and mallocsize, which wouldn't solve this problem. Right, we'd still need a cargo feature to disable the use of heapsize, if I'm understanding what you're proposing.
Comment 21•7 years ago
|
||
I'm going to assign this bug to you since I'm not really doing anything with it at this point and you are :)
Assignee: bugmail → n.nethercote
Comment 23•7 years ago
|
||
Is there a workaround available for this problem?
Comment 24•6 years ago
|
||
The next update of WebRender in Gecko should include https://github.com/servo/webrender/pull/2192, remove heapsize from the dependency graph, and hopefully fix this bug. (heapsize is replaced with malloc_size_of, which does not link to jemalloc but instead expect users to provide a callback: https://github.com/servo/servo/tree/master/components/malloc_size_of)
Updated•6 years ago
|
Depends on: 1424280
See Also: → https://github.com/servo/webrender/pull/2192
Comment 25•6 years ago
|
||
Jonathan, can you check that this is fixed in central?
Flags: needinfo?(jwatt)
Reporter | ||
Comment 26•6 years ago
|
||
Yes, it's fixed now. \o/ Thanks!
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(jwatt)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•