Closed
Bug 1417958
Opened 7 years ago
Closed 6 years ago
Super long incremental link once again
Categories
(Firefox Build System :: General, defect)
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: away)
Details
(Keywords: regression)
Attachments
(1 file)
1.82 KB,
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
OS: Unspecified → Windows
Comment 1•7 years ago
|
||
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.
Comment 3•7 years ago
|
||
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).
Reporter | ||
Comment 8•6 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!
Reporter | ||
Comment 9•6 years ago
|
||
I've lodged https://developercommunity.visualstudio.com/content/problem/152301/extremely-slow-incremental-linkage-following-upgra.html
Assignee | ||
Comment 10•6 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•6 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•6 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 13•6 years ago
|
||
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 | ||
Comment 14•6 years ago
|
||
https://hg.mozilla.org/try/rev/c6c066014da2a5cb5efe98f6d285f833d34d910d
Keywords: checkin-needed
Updated•6 years ago
|
Assignee: nobody → dmajor
Comment 15•6 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
Comment 16•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a1b0ad890d98
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 17•6 years ago
|
||
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•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/bacdbc0d22f0
Whiteboard: [checkin-needed-beta, a=npotb]
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•