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)

enhancement
Not set
normal

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.
Blocks: 1350582
I'm hitting the same problem. I'm using --disable-optimize with non-debug builds for the same reasons as Jonathan.
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?
Stylo disables the heapsize crate. WebRender should probably add a similar Cargo feature to do so as well.
(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)
I can own this, sure.
Assignee: nobody → bugmail
Flags: needinfo?(bugmail)
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)
> 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)
(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.
(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...
> 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.
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)
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.
> Then every crate that uses heapsize will need to be updated.

And there's something like a dozen of them, alas.
Whiteboard: [workaround: ac_add_options --disable-webrender]
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.
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.
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
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
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)
> 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)
(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.
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
Is there a workaround available for this problem?
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)
Jonathan, can you check that this is fixed in central?
Flags: needinfo?(jwatt)
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.