Closed
Bug 922912
Opened 11 years ago
Closed 10 years ago
Fold gkmedias.dll back into xul.dll
Categories
(Firefox Build System :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla36
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(2 files, 3 obsolete files)
942 bytes,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.17 KB,
patch
|
mshal
:
review+
|
Details | Diff | Splinter Review |
Since memory use has been going down thanks to include reduction, and since we're (trying to) fold mozjs.dll back into xul.dll in bug 915735, I figured I'd see what folding gkmedias.dll back into xul.dll does to memory use during linking. And it makes it grow by about 320MB, which is too much to do it now, but we may want to do it in the not so distant future when we've switched to vs2013, so I'm going to attach a patch that does it here, for posterity, since i've done it for testing anyways.
Assignee | ||
Comment 1•11 years ago
|
||
Comment 2•11 years ago
|
||
I talked to glandium about this on IRC. I don't think that we should do this before switching to VS2013, unless my investigations in bug 915735 lead to the conclusion that folding mozjs into xul.dll is not an option. In that case, we might consider doing this.
Comment 3•11 years ago
|
||
According to comments in bug 922441, the amd64_x86 toolchain didn't help us :(
No longer depends on: VC12
Assignee | ||
Comment 4•11 years ago
|
||
This is refreshed for current trunk. I sent it to try on top of a recent m-c. linker max vsize: - for the m-c build: 2780344320 (yeah, unified sources have also done that) - with the patch: 3071430656 I'd say it's the right time to do this.
Attachment #8335636 -
Flags: review?(gps)
Assignee | ||
Updated•11 years ago
|
Attachment #812910 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•11 years ago
|
||
One thing to consider though. The compilation step of the builds took: 2 hrs, 28 mins, 11 secs on m-c 2 hrs, 47 mins, 50 secs with the patch. That's 20 minutes more.
Assignee | ||
Comment 6•11 years ago
|
||
Another data point, looking at an arbitrary changeset before the unified sources work: 2 hrs, 37 mins, 5 secs for 361a24da7112. So we won 10 minutes, and would lose 20.
Comment 7•11 years ago
|
||
We should not take this patch at least until bug 915735 is resolved one way or another.
Depends on: 915735
Comment 8•11 years ago
|
||
I think we should also carefully assess how this influences startup time. It feels to me like gkmedias is code that we should not need at startup but when we fold it into libxul, we need to load it before doing anything meaningful, right?
Comment 9•11 years ago
|
||
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #8) > I think we should also carefully assess how this influences startup time. It > feels to me like gkmedias is code that we should not need at startup but > when we fold it into libxul, we need to load it before doing anything > meaningful, right? We currently preload gkmedias.dll so this is moot.
Assignee | ||
Updated•11 years ago
|
Attachment #8335636 -
Flags: review?(gps)
Comment 10•10 years ago
|
||
OK, this can land now I guess.
Assignee | ||
Comment 11•10 years ago
|
||
Patch is now much simpler, thanks to bug 942043. I think it's still useful to keep the option of making gkmedias separated in case we want to go back. We can remove in a followup when we're confident it will stick.
Attachment #8344008 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8335636 -
Attachment is obsolete: true
Comment 12•10 years ago
|
||
Can you please write to dev.platform before landing this? Thanks!
Comment 13•10 years ago
|
||
Comment on attachment 8344008 [details] [diff] [review] Fold gkmedias.dll back into xul.dll Review of attachment 8344008 [details] [diff] [review]: ----------------------------------------------------------------- +1 for unifying platform configurations. I'm not thrilled about increasing PGO build times and memory usage. But PGO already sucks. We should trigger a number of Talos runs before and after this patch to ensure it doesn't regress anything.
Attachment #8344008 -
Flags: review?(gps) → review+
Comment 14•10 years ago
|
||
IIUC, one of the big reasons this is possible is because of the massive reduction in PGO linker memory usage we've realized from unified builds. However, we only have unified builds enabled on trunk. What happens on merge day when Aurora isn't using unified builds?
Comment 15•10 years ago
|
||
(In reply to comment #14) > IIUC, one of the big reasons this is possible is because of the massive > reduction in PGO linker memory usage we've realized from unified builds. > However, we only have unified builds enabled on trunk. What happens on merge > day when Aurora isn't using unified builds? This is a good point. We should measure the amount of additional memory usage impact on non-unified PGO builds before and after this change.
Assignee | ||
Comment 16•10 years ago
|
||
VS2013 can sustain both js and gkmedias folded back into xul.dll. The resulting xul.dll is about 1.8MB smaller than the sum of all the dlls that end up being folded (that includes icu, too) Sadly, this only makes 124K difference on the installer size. The one downside is that this makes the build about 13 minutes slower. This is better than the 20 minutes more VS2010 was spending just by folding gkmedias, though. And in absolute terms, it's probably something we can just accept ; hear this: - VS2010 PGO builds's compile step was taking 2h48 (at least it did on https://treeherder.mozilla.org/ui/logviewer.html#?job_id=477231&repo=mozilla-central) - VS2013 PGO builds's compile step now takes 1h30 (well, a try pgo build https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2468586&repo=try) - VS2013 PGO build with both js and gkmedias folded back into xul.dll compile step took 1h43 on try (https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2468669&repo=try) That's still 1h05 faster than VS2010 builds were!
Attachment #8506074 -
Flags: review?(gps)
Assignee | ||
Updated•10 years ago
|
Attachment #8344008 -
Attachment is obsolete: true
Comment 17•10 years ago
|
||
Comment on attachment 8506074 [details] [diff] [review] Fold gkmedias.dll back into xul.dll r=me whenever this works :)
Attachment #8506074 -
Flags: review?(gps) → review+
Assignee | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/151d7871715c
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/151d7871715c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 20•10 years ago
|
||
Should removed-files.in have been updated with this change?
Flags: needinfo?(mh+mozilla)
Comment 21•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20) > Should removed-files.in have been updated with this change? I think so...
Assignee | ||
Comment 22•10 years ago
|
||
Indeed. I still don't understand why this file is still not generated based on actual contents.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8507279 -
Flags: review?(mshal)
Updated•10 years ago
|
Attachment #8507279 -
Flags: review?(mshal) → review+
Assignee | ||
Comment 24•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/96baea8ac9ab
Assignee | ||
Comment 25•10 years ago
|
||
Backed out the removed-files.in part, as it appears it's not necessary anymore https://hg.mozilla.org/integration/mozilla-inbound/rev/536c500f8e76
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•