Closed Bug 1417958 Opened 3 years ago Closed 3 years ago

Super long incremental link once again

Categories

(Firefox Build System :: General, defect)

Unspecified
Windows
defect
Not set
normal

Tracking

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

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: jya, Assigned: dmajor)

Details

(Keywords: regression)

Attachments

(1 file)

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.
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)
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)
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.
Process Monitor says link.exe is deleting xul.ilk (the incremental linking file), and launching a child link.exe that links everything from scratch. :-(
It happens for mozglue.ilk as well (which ought to be less painful to debug).
Same for qipcap64.ilk (even easier).
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!
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
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).
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
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
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
We should take this on Beta too.
Whiteboard: [checkin-needed-beta, a=npotb]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.