Closed Bug 1173683 Opened 6 years ago Closed 6 years ago

Unfold mozalloc for firefox 39

Categories

(Core :: Memory Allocator, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 + fixed
firefox40 + wontfix
firefox41 + wontfix

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(2 files)

No description provided.
Approval Request Comment
[Feature/regressing bug #]:
Bug 868814 folded mozalloc into mozglue. While it works fine, there are some complications for third party components on Windows. One is crashes, but that might end up just being a linking problem, and another is bug 1168291. While that bug has a patch, there are redistribution issues that we need to sort out first. So until we have those issues sorted out, I'd rather backout to unblock those people building third party components.

[User impact if declined]:
Difficulty to build third party components on Windows.

[Describe test coverage new/current, TreeHerder]:
I did a try build (https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c90ceb86feb) which:
- fails e10s tests but beta doesn't run them (and doesn't have e10s)
- failed to build on mac because of bug 1154377
- failed to build on windows because of a conflict with bug 1147447 that will require an additional patch
- failed mochitest-push, but beta doesn't run them
- failed b2g emulator mochitests for some reason (looks like try just doesn't like beta)
- failed gaia tests for whatever reason, but beta doesn't run them

[Risks and why]: 
On non-Windows, this gets us back to where we were before bug 868814 wrt mozalloc, so there's no risk at all. On Windows, this would get us back to the same, if the fix for bug 1147447 wasn't getting in the way. The fix is hopefully straightforward: it should only require adding mozalloc as a dependency to libEGL and libGLES. Hopefully, whether /that/ causes problems should be visible on try.

[String/UUID change made/needed]: None
Assignee: nobody → mh+mozilla
Attachment #8620850 - Flags: approval-mozilla-beta?
Forgot to mention: there are things that piled up on top of those changes from bug 868814 that make it much harder to possibly backout from aurora and central. IOW, those issues will have to be figured out in the next 9 weeks.
Here's the rationale:
- before bug 1147447, libEGL and libGLESv2 were using jemalloc for malloc/free via mozglue (which I don't think they were using directly) and the system allocator operator new/delete. The latter made OOM dealt with C++ exceptions instead of our "standard" OOM mechanism.
- after bug 1147447, libEGL and libGLESv2 use jemalloc for malloc/free/operator new/delete via mozglue. Obviously, we want to keep this property.
- backing out bug 868814 means the operator new->moz_xmalloc->malloc path changes from being all in mozglue to being split between mozalloc and mozglue, with the entry point in mozalloc.
So, explicitly link those libraries against mozalloc. The added runtime dependency shouldn't be a problem.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4df39dac6801
Attachment #8621255 - Flags: review?(n.nethercote)
Comment on attachment 8621255 [details] [diff] [review]
Link libEGL and libGLESv2 against mozalloc

That's the additional patch that makes the backout build on windows.
Attachment #8621255 - Flags: approval-mozilla-beta?
Mike, how does this issue manifest? Can we reproduce it? (To verify that this works)
The issue is bug 1168291. People can't use the SDK because bug 868814 removed mozalloc.lib but failed to add mozcrt.lib which is needed instead of it, but whether we can actually redistribute mozcrt.lib is not sure yet (while the problem doesn't exist with mozalloc.lib). Verifying this works would be checking that mozalloc.lib is in the sdk.
Comment on attachment 8621255 [details] [diff] [review]
Link libEGL and libGLESv2 against mozalloc

Approved for uplift to beta
Attachment #8621255 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Well, approved once the 2nd patch gets reviewed.
Mike can you follow up? This can still go into beta 7.
Flags: needinfo?(mh+mozilla)
(In reply to Liz Henry (:lizzard) from comment #8)
> Well, approved once the 2nd patch gets reviewed.

What about the first patch?
Flags: needinfo?(mh+mozilla)
Comment on attachment 8621255 [details] [diff] [review]
Link libEGL and libGLESv2 against mozalloc

Review of attachment 8621255 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me
Attachment #8621255 - Flags: review?(n.nethercote) → review+
Flags: needinfo?(lhenry)
I must have missed this too - I thought the first patch already had a review. Usually people don't request uplift until there's an r+
Flags: needinfo?(lhenry)
Blocks: 1170141
Comment on attachment 8620850 [details] [diff] [review]
Backout changesets beed1c584a22, 9b2413915a5e (bug 868814), 1f34ebc9eb00, 80555e0558a3 (bug 1141660), 22aea188dac5 (bug 1141731) to give us more time to figure the situation with the SDK.

Approved for uplift to beta
Attachment #8620850 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1176599
You need to log in before you can comment on or make changes to this bug.