Fold gkmedias.dll back into xul.dll

RESOLVED FIXED in mozilla36

Status

()

Core
Build Config
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla36
All
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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

4 years ago
Created attachment 812910 [details] [diff] [review]
Fold gkmedias.dll back into xul.dll

Comment 2

4 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.
According to comments in bug 922441, the amd64_x86 toolchain didn't help us :(
No longer depends on: 914596
(Assignee)

Comment 4

4 years ago
Created attachment 8335636 [details] [diff] [review]
Fold gkmedias.dll back into xul.dll

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

4 years ago
Attachment #812910 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 5

4 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

4 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

4 years ago
We should not take this patch at least until bug 915735 is resolved one way or another.
Depends on: 915735

Comment 8

4 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

4 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

4 years ago
Attachment #8335636 - Flags: review?(gps)

Comment 10

4 years ago
OK, this can land now I guess.
(Assignee)

Comment 11

4 years ago
Created attachment 8344008 [details] [diff] [review]
Fold gkmedias.dll back into xul.dll

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

4 years ago
Attachment #8335636 - Attachment is obsolete: true

Comment 12

4 years ago
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?

Comment 15

4 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

3 years ago
Created attachment 8506074 [details] [diff] [review]
Fold gkmedias.dll back into xul.dll

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

3 years ago
Attachment #8344008 - Attachment is obsolete: true

Comment 17

3 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

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/151d7871715c

Updated

3 years ago
Depends on: 914596
https://hg.mozilla.org/mozilla-central/rev/151d7871715c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Should removed-files.in have been updated with this change?
Flags: needinfo?(mh+mozilla)

Comment 21

3 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

3 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

3 years ago
Created attachment 8507279 [details] [diff] [review]
Update removed-files.in to account for the removal of gkmedias.dll
Attachment #8507279 - Flags: review?(mshal)

Updated

3 years ago
Attachment #8507279 - Flags: review?(mshal) → review+
(Assignee)

Comment 24

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/96baea8ac9ab
(Assignee)

Comment 25

3 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
Depends on: 1086539

Updated

3 years ago
Depends on: 1092151
Blocks: 1170983
You need to log in before you can comment on or make changes to this bug.