Super long incremental link once again

RESOLVED FIXED in Firefox 58

Status

()

Core
Build Config
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: jya, Assigned: dmajor (offline))

Tracking

({regression})

Trunk
mozilla59
Unspecified
Windows
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 fixed, firefox59 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 months ago
Bug 1411712 broke incremental linkage (that was fixed in bug 1341504)

It causes every rebuild no matter how small, to take over 2.5 minutes.

Bug 1411712 also makes it impossible to debug what's going on, not showing anything with ./mach build --verbose.
(Reporter)

Updated

2 months ago
OS: Unspecified → Windows
Checking locally adding '-OPT:REF,ICF' or not appears to be correctly keyed off of --disable-optimize.

What is in your mozconfig?
Flags: needinfo?(jyavenard)
(Assignee)

Comment 2

2 months ago
jya and I have been discussing this on IRC. He's building --disable-optimize (so no -OPT:REF,ICF), and recently upgraded to VS2017. I think this is VS2017's fault. :-(

I just tried these steps comparing --with-visual-studio-version={2015,2017}:
1. Clobber build of --disable-optimize
2. touch toolkit/crashreporter/nsExceptionHandler.cpp
3. mach build binaries

My VS2015 build does step #3 in seconds. VS2017 takes minutes.
That is :-(. Let me know if it looks like there's anything going wrong with our flags.
No longer blocks: 1411712
Flags: needinfo?(jyavenard)
(Assignee)

Comment 4

2 months ago
FWIW the release notes for Visual Studio 2017 15.5 Preview say: "We made fixes to incremental linking. Incremental linking will never be slower than full linking."

So maybe they've fixed bug 1346069. But this problem still exists in the 15.5 Preview: it's not that incremental linking is slower than full linking; they're the same speed. Or maybe another way to say it is that incremental linking is disabled.
(Assignee)

Comment 5

2 months ago
Process Monitor says link.exe is deleting xul.ilk (the incremental linking file), and launching a child link.exe that links everything from scratch. :-(
(Assignee)

Comment 6

2 months ago
It happens for mozglue.ilk as well (which ought to be less painful to debug).
(Assignee)

Comment 7

2 months ago
Same for qipcap64.ilk (even easier).
(Reporter)

Comment 8

2 months ago
Humm... yes you're right...

This is using Visual Studio 2017 15.3.5

 0:17.23 Unified_cpp_media_platforms_wmf0.cpp
 0:17.23 dom_media_platforms_wmf.lib.desc
 0:17.50 xul_s.lib.desc
 0:17.50 xul.dll
 0:25.32 Your build was successful!

Using VS 2017 15.4.4
 0:17.95 Unified_cpp_media_platforms_wmf0.cpp
 0:17.95 dom_media_platforms_wmf.lib.desc
 0:18.22 xul_s.lib.desc
 0:18.22 xul.dll
 2:43.78    Creating library xul.lib and object xul.exp
 2:43.78
 2:45.86 Overall system resources - Wall time: 165s; CPU: 22%; Read bytes: 6549324288; Write bytes: 1442407936; Read time: 4800788; Write time: 3760119
 2:45.89 Your build was successful!
(Assignee)

Comment 10

2 months ago
link -verbose says:

LINK :Removed file: ..\..\..\security\nss3.lib
LINK :Removed file: ..\..\..\mozglue\build\mozglue.lib
LINK :New file: ../../../security/nss3.lib
LINK :New file: ../../../mozglue/build/mozglue.lib
LINK : object file added; performing full link
(Assignee)

Comment 11

2 months ago
This is https://developercommunity.visualstudio.com/content/problem/142544/incremental-linking-regresion-in-154.html which is supposedly fixed in a pending release (but not the 15.5 Preview that I tried yesterday).
(Assignee)

Comment 12

2 months ago
Created attachment 8929514 [details] [diff] [review]
Normalize lib paths to appease VS2017's incremental linking

In VS2017 version 15.4, the incremental linker became intolerant of path-separator mismatches. Internally, it records the files it previously linked in terms of their Windows-style paths like `..\foo.lib`. When we run a subsequent link against a Unix-style path like `../foo.lib`, the linker believes that we've added a "new" library and therefore it must do a full re-link.

To work around this, we need to pass all libraries (and $LIB) with Windows-style paths.
Attachment #8929514 - Flags: review?(core-build-config-reviews)
Comment on attachment 8929514 [details] [diff] [review]
Normalize lib paths to appease VS2017's incremental linking

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

OK, this makes some sense.  If it's working for you locally and green on try, bombs away!
Attachment #8929514 - Flags: review?(core-build-config-reviews) → review+
Assignee: nobody → dmajor

Comment 15

2 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1b0ad890d98
Normalize lib paths to appease VS2017's incremental linking. r=nalexander
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a1b0ad890d98
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
We should take this on Beta too.
status-firefox57: --- → unaffected
status-firefox58: --- → affected
status-firefox-esr52: --- → unaffected
Whiteboard: [checkin-needed-beta, a=npotb]

Comment 18

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/bacdbc0d22f0
status-firefox58: affected → fixed
Whiteboard: [checkin-needed-beta, a=npotb]
You need to log in before you can comment on or make changes to this bug.