Super long incremental link once again

RESOLVED FIXED in Firefox 58

Status

defect
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: jya, Assigned: dmajor)

Tracking

({regression})

Trunk
mozilla59
Unspecified
Windows

Firefox Tracking Flags

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

Details

Attachments

(1 attachment)

Reporter

Description

2 years 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 years 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 years 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 years 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 years 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 years ago
It happens for mozglue.ilk as well (which ought to be less painful to debug).
Assignee

Comment 7

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

Comment 8

2 years 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 years 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 years 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 years ago
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 years 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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
We should take this on Beta too.
Whiteboard: [checkin-needed-beta, a=npotb]

Updated

Last year
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.