Closed Bug 1322703 Opened 7 years ago Closed 5 years ago

Use -Fd to specify unique PDB filename per-compile for MSVC

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox54 fixed)

RESOLVED WONTFIX
Tracking Status
firefox54 --- fixed

People

(Reporter: ted, Unassigned)

References

Details

Attachments

(1 file)

We're currently not doing this, and because of that we have to use -Z7 for sccache, but now that NSS is using the same build system as the rest of Gecko we can do this universally.
Blocks: 1318370
Clarifying, we currently just use `-Fdgenerated.pdb`, so we share a single PDB file per-directory:
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/config/rules.mk#156

We used to use a single PDB file per-source file at some point, but we changed that with bug 462740 and I don't really remember why, despite reviewing the patch. Older versions of MSVC used to not be able to handle concurrent PDB access, but newer versions will route everything through mspdbserv. I can only suspect that maybe the build is slower if we generate a single PDB per source file and then force the linker to link them, vs. generating a single PDB file, letting mspdbserv do the work and not the linker?

In any event, sccache can't cache the current state of affairs because multiple compiles will affect the same output PDB file, so we override this in mozconfig.cache:
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/build/mozconfig.cache#134

I doubt using a separate PDB per-compile would be any worse than using -Z7, where the linker has to link the debug info anyway. I can test build timings on my local machine and see if it has an impact there, and if so we can limit this to only happen with sccache.
Summary: Use -Fd to specify PDB filename for MSVC → Use -Fd to specify unique PDB filename per-compile for MSVC
The amount of CPU time spent in mspdbserv.exe during builds is non-trivial. I wouldn't at all be surprised if there were contention on single PDB files slowing down builds. Then again, I would expect Microsoft to have a handle on this problem, since the default MSVC configuration is to share a single PDB file.
Assignee: nobody → ted
We might be able to undo bug 915973 after this change.
My try pushes for this change are on bug 1318370.
We will be able to remove -FS into old-configure.in if we don't use per directory PDB file.  But I don't test yet.
Comment on attachment 8830678 [details]
bug 1322703 - use -Fd to specify unique PDB filename per-object-file for MSVC.

https://reviewboard.mozilla.org/r/107410/#review109800
Attachment #8830678 - Flags: review?(mh+mozilla) → review+
Pushed by tmielczarek@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7a2f7ff5e87
use -Fd to specify unique PDB filename per-object-file for MSVC. r=glandium
https://hg.mozilla.org/mozilla-central/rev/b7a2f7ff5e87
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
My opt xul.dll is taking nearly 4 minutes to link today, compared to about 2 minutes last week, and I bet this change is the culprit.
(In reply to David Major [:dmajor] from comment #10)
> My opt xul.dll is taking nearly 4 minutes to link today, compared to about 2
> minutes last week, and I bet this change is the culprit.

If the purpose of this change was to please ccache, does it need to happen in non-ccache builds?
Depends on: 1340395
Blocks: 1340393
(In reply to David Major [:dmajor] from comment #11)
> (In reply to David Major [:dmajor] from comment #10)
> > My opt xul.dll is taking nearly 4 minutes to link today, compared to about 2
> > minutes last week, and I bet this change is the culprit.
> 
> If the purpose of this change was to please ccache, does it need to happen
> in non-ccache builds?

It doesn't, but because this code lived in the recursive make backend it made things a lot simpler to have a single codepath there. We can change that to make life better. (but also we should fix bug 1341504!)
With this change, it's impossible to link Thunderbird on a machine with 8GB RAM. The linker fails trying to pull together 3587 PDB files, see bug 1340395. There someone tried with a 16GB machine and failed; in in bug 1342617 someone also failed, but I don't know how much physical RAM they have. Alice?

Undoing this change in a local config links again with only 722 PDB files. We don't understand how build bot builds work since those servers apparently have 16GB as well.
Flags: needinfo?(ted)
(In reply to Jorg K (GMT+1) from comment #13)
> With this change, it's impossible to link Thunderbird on a machine with 8GB
> RAM. The linker fails trying to pull together 3587 PDB files, see bug
> 1340395. There someone tried with a 16GB machine and failed; in in bug
> 1342617 someone also failed, but I don't know how much physical RAM they
> have. Alice?
> 

Physical RAM is 8GB on my Windows10 64bit PC.
Blocks: 1342827
Depends on: 1343155
Given all the headaches this is causing I think we should back this out for now. I can change the patch so we only do this when  sccache is enabled. Alternately, we could revisit this patch entirely and just make sccache builds use the existing -Z7 behavior by default for local builds, which might be less bad given the linker memory usage here.
Flags: needinfo?(ted)
Not sure the linker memory usage with -Z7 would be lower. There should be about the same amount of debug info, except they would be in the .obj files instead of separate pdb files. But maybe the linker treats pdb files very differently, who knows.
I backed the change out for now. We'll figure out something better here. Sorry for the inconvenience.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla54 → ---
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4aa866ebfeaa
use -Fd to specify unique PDB filename per-object-file for MSVC. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/af1edb14a707
Backed out changeset 4aa866ebfeaa
Product: Core → Firefox Build System

I landed a patch in bug 1506848, that's good enough.

Assignee: ted → nobody
Status: REOPENED → RESOLVED
Closed: 7 years ago5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.