Closed Bug 922912 Opened 6 years ago Closed 5 years ago

Fold gkmedias.dll back into xul.dll

Categories

(Firefox Build System :: General, defect)

All
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
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.
According to comments in bug 922441, the amd64_x86 toolchain didn't help us :(
No longer depends on: VC12
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)
Attachment #812910 - Attachment is obsolete: true
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
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.
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.
We should not take this patch at least until bug 915735 is resolved one way or another.
Depends on: 915735
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?
(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.
Attachment #8335636 - Flags: review?(gps)
OK, this can land now I guess.
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)
Attachment #8335636 - Attachment is obsolete: true
Can you please write to dev.platform before landing this?  Thanks!
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+
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?
(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.
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)
Attachment #8344008 - Attachment is obsolete: true
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+
Depends on: VC12
https://hg.mozilla.org/mozilla-central/rev/151d7871715c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Should removed-files.in have been updated with this change?
Flags: needinfo?(mh+mozilla)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20)
> Should removed-files.in have been updated with this change?

I think so...
Indeed. I still don't understand why this file is still not generated based on actual contents.
Flags: needinfo?(mh+mozilla)
Attachment #8507279 - Flags: review?(mshal) → review+
Backed out the removed-files.in part, as it appears it's not necessary anymore
https://hg.mozilla.org/integration/mozilla-inbound/rev/536c500f8e76
Depends on: 1092151
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.