Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2331373006:
Enable incremental linking in release component builds (Closed)
|
Created:
4 years, 3 months ago by brucedawson Modified:
4 years, 3 months ago CC:
chromium-reviews, blink-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable incremental linking in release component builds
Component builds are, by definition, not optimized for maximum
performance. So incremental linking - which trades a little bit of
performance for much faster build times - is appropriate in release
component builds, as well as in debug builds.
This change enables incremental linking in release component builds.
This also requires turning off /OPT:ICF and /PROFILE in component
builds because they are incompatible with /incremental, but they are
performance related flags so they are also not important for component
builds.
blink_core.dll cannot be incrementally linked in debug x64 component
builds, but it *can* be incrementally linked in release x64 component
builds (and debug/release x86 component builds), and this change
enables incremental linking of blink_core.dll in all supported
configurations.
This drops the chrome rebuild time after touching
third_party\WebKit\Source\core\html\AutoplayExperimentHelper.cpp from
~260 seconds to 9 seconds. The chrome rebuild time after touching
base\win\registry.cc drops from ~200 seconds to less than 2 seconds,
with 21 of 24 link steps skipped entirely.
This also fixes some /INCREMENTAL /OPT:ICF mismatch warnings.
BUG=608801, 621236
Committed: https://crrev.com/7bf1119ce51d5e5b489c780bbacc332eabdd6f07
Cr-Commit-Position: refs/heads/master@{#418660}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Tweak logic and add bug link #
Messages
Total messages: 23 (17 generated)
The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
https://codereview.chromium.org/2331373006/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2331373006/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1244: common_optimize_on_ldflags += [ "/OPT:ICF" ] # Redundant COMDAT folding. Does this fix https://bugs.chromium.org/p/chromium/issues/detail?id=621236 ? Or is that still broken for non-component builds? https://codereview.chromium.org/2331373006/diff/1/build/config/win/BUILD.gn File build/config/win/BUILD.gn (right): https://codereview.chromium.org/2331373006/diff/1/build/config/win/BUILD.gn#n... build/config/win/BUILD.gn:105: if (!is_debug && !is_win_fastlink && !is_clang && !is_component_build) { Should this match the condition on line 317? I'm wondering if we should still have this flag for the "is_syzyasan" case since we don't incrementally link in that case anyway. https://codereview.chromium.org/2331373006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/BUILD.gn (right): https://codereview.chromium.org/2331373006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/BUILD.gn:183: # link silently fails. Therefore 32-bit builds or 64-bit release builds Nit: Should this reference whatever Microsoft issue got filed about these large .ilk files, so later we'll know if we can re-enable this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Enable incremental linking in release component builds Component builds are, by definition, not optimized for maximum performance. So incremental linking - which trades a little bit of performance for much faster build times - is appropriate in release component builds, as well as in debug builds. This change enables incremental linking in release component builds. This also requires turning off /OPT:ICF and /PROFILE in component builds because they are incompatible with /incremental, but they are performance related flags so they are also not important for component builds. blink_core.dll cannot be incrementally linked in debug x64 component builds, but it *can* be incrementally linked in release x64 component builds, and this change enables that. This drops the rebuild time after touching third_party\WebKit\Source\core\html\AutoplayExperimentHelper.cpp from <unmeasurably long> to 9 seconds. ========== to ========== Enable incremental linking in release component builds Component builds are, by definition, not optimized for maximum performance. So incremental linking - which trades a little bit of performance for much faster build times - is appropriate in release component builds, as well as in debug builds. This change enables incremental linking in release component builds. This also requires turning off /OPT:ICF and /PROFILE in component builds because they are incompatible with /incremental, but they are performance related flags so they are also not important for component builds. blink_core.dll cannot be incrementally linked in debug x64 component builds, but it *can* be incrementally linked in release x64 component builds, and this change enables that. This drops the rebuild time after touching third_party\WebKit\Source\core\html\AutoplayExperimentHelper.cpp from very long to 9 seconds. The rebuild time after touching base\win\registry.cc drops from very long to less than 2 seconds. This also fixes some /INCREMENTAL /OPT:ICF mismatch warnings. BUG=621236 ==========
The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
brucedawson@chromium.org changed reviewers: + dpranke@chromium.org
I made some changes based on review feedback - thanks for looking. Dirk - PTAL. I think that component builds should be incremental even in release builds, and the differences in build times can be enormous. https://codereview.chromium.org/2331373006/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2331373006/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1244: common_optimize_on_ldflags += [ "/OPT:ICF" ] # Redundant COMDAT folding. On 2016/09/14 01:15:17, Peter Kasting wrote: > Does this fix https://bugs.chromium.org/p/chromium/issues/detail?id=621236 ? Or > is that still broken for non-component builds? Yes it does fix that. I am not clear on where incremental was getting enabled for mksnapshot etc. but this change does avoid the contradiction and the warnings. https://codereview.chromium.org/2331373006/diff/1/build/config/win/BUILD.gn File build/config/win/BUILD.gn (right): https://codereview.chromium.org/2331373006/diff/1/build/config/win/BUILD.gn#n... build/config/win/BUILD.gn:105: if (!is_debug && !is_win_fastlink && !is_clang && !is_component_build) { On 2016/09/14 01:15:17, Peter Kasting wrote: > Should this match the condition on line 317? I'm wondering if we should still > have this flag for the "is_syzyasan" case since we don't incrementally link in > that case anyway. I had been thinking that it didn't really matter - not all combinations of the flags are supported or used - but special casing syzyasan is easy enough. Done. https://codereview.chromium.org/2331373006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/BUILD.gn (right): https://codereview.chromium.org/2331373006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/BUILD.gn:183: # link silently fails. Therefore 32-bit builds or 64-bit release builds On 2016/09/14 01:15:17, Peter Kasting wrote: > Nit: Should this reference whatever Microsoft issue got filed about these large > .ilk files, so later we'll know if we can re-enable this? Done.
Description was changed from ========== Enable incremental linking in release component builds Component builds are, by definition, not optimized for maximum performance. So incremental linking - which trades a little bit of performance for much faster build times - is appropriate in release component builds, as well as in debug builds. This change enables incremental linking in release component builds. This also requires turning off /OPT:ICF and /PROFILE in component builds because they are incompatible with /incremental, but they are performance related flags so they are also not important for component builds. blink_core.dll cannot be incrementally linked in debug x64 component builds, but it *can* be incrementally linked in release x64 component builds, and this change enables that. This drops the rebuild time after touching third_party\WebKit\Source\core\html\AutoplayExperimentHelper.cpp from very long to 9 seconds. The rebuild time after touching base\win\registry.cc drops from very long to less than 2 seconds. This also fixes some /INCREMENTAL /OPT:ICF mismatch warnings. BUG=621236 ========== to ========== Enable incremental linking in release component builds Component builds are, by definition, not optimized for maximum performance. So incremental linking - which trades a little bit of performance for much faster build times - is appropriate in release component builds, as well as in debug builds. This change enables incremental linking in release component builds. This also requires turning off /OPT:ICF and /PROFILE in component builds because they are incompatible with /incremental, but they are performance related flags so they are also not important for component builds. blink_core.dll cannot be incrementally linked in debug x64 component builds, but it *can* be incrementally linked in release x64 component builds (and debug/release x86 component builds), and this change enables incremental linking of blink_core.dll in all supported configurations. This drops the chrome rebuild time after touching third_party\WebKit\Source\core\html\AutoplayExperimentHelper.cpp from ~260 seconds to 9 seconds. The chrome rebuild time after touching base\win\registry.cc drops from ~200 seconds to less than 2 seconds. This also fixes some /INCREMENTAL /OPT:ICF mismatch warnings. BUG=621236 ==========
lgtm
Description was changed from ========== Enable incremental linking in release component builds Component builds are, by definition, not optimized for maximum performance. So incremental linking - which trades a little bit of performance for much faster build times - is appropriate in release component builds, as well as in debug builds. This change enables incremental linking in release component builds. This also requires turning off /OPT:ICF and /PROFILE in component builds because they are incompatible with /incremental, but they are performance related flags so they are also not important for component builds. blink_core.dll cannot be incrementally linked in debug x64 component builds, but it *can* be incrementally linked in release x64 component builds (and debug/release x86 component builds), and this change enables incremental linking of blink_core.dll in all supported configurations. This drops the chrome rebuild time after touching third_party\WebKit\Source\core\html\AutoplayExperimentHelper.cpp from ~260 seconds to 9 seconds. The chrome rebuild time after touching base\win\registry.cc drops from ~200 seconds to less than 2 seconds. This also fixes some /INCREMENTAL /OPT:ICF mismatch warnings. BUG=621236 ========== to ========== Enable incremental linking in release component builds Component builds are, by definition, not optimized for maximum performance. So incremental linking - which trades a little bit of performance for much faster build times - is appropriate in release component builds, as well as in debug builds. This change enables incremental linking in release component builds. This also requires turning off /OPT:ICF and /PROFILE in component builds because they are incompatible with /incremental, but they are performance related flags so they are also not important for component builds. blink_core.dll cannot be incrementally linked in debug x64 component builds, but it *can* be incrementally linked in release x64 component builds (and debug/release x86 component builds), and this change enables incremental linking of blink_core.dll in all supported configurations. This drops the chrome rebuild time after touching third_party\WebKit\Source\core\html\AutoplayExperimentHelper.cpp from ~260 seconds to 9 seconds. The chrome rebuild time after touching base\win\registry.cc drops from ~200 seconds to less than 2 seconds, with 21 of 24 link steps skipped entirely. This also fixes some /INCREMENTAL /OPT:ICF mismatch warnings. BUG=621236 ==========
Description was changed from ========== Enable incremental linking in release component builds Component builds are, by definition, not optimized for maximum performance. So incremental linking - which trades a little bit of performance for much faster build times - is appropriate in release component builds, as well as in debug builds. This change enables incremental linking in release component builds. This also requires turning off /OPT:ICF and /PROFILE in component builds because they are incompatible with /incremental, but they are performance related flags so they are also not important for component builds. blink_core.dll cannot be incrementally linked in debug x64 component builds, but it *can* be incrementally linked in release x64 component builds (and debug/release x86 component builds), and this change enables incremental linking of blink_core.dll in all supported configurations. This drops the chrome rebuild time after touching third_party\WebKit\Source\core\html\AutoplayExperimentHelper.cpp from ~260 seconds to 9 seconds. The chrome rebuild time after touching base\win\registry.cc drops from ~200 seconds to less than 2 seconds, with 21 of 24 link steps skipped entirely. This also fixes some /INCREMENTAL /OPT:ICF mismatch warnings. BUG=621236 ========== to ========== Enable incremental linking in release component builds Component builds are, by definition, not optimized for maximum performance. So incremental linking - which trades a little bit of performance for much faster build times - is appropriate in release component builds, as well as in debug builds. This change enables incremental linking in release component builds. This also requires turning off /OPT:ICF and /PROFILE in component builds because they are incompatible with /incremental, but they are performance related flags so they are also not important for component builds. blink_core.dll cannot be incrementally linked in debug x64 component builds, but it *can* be incrementally linked in release x64 component builds (and debug/release x86 component builds), and this change enables incremental linking of blink_core.dll in all supported configurations. This drops the chrome rebuild time after touching third_party\WebKit\Source\core\html\AutoplayExperimentHelper.cpp from ~260 seconds to 9 seconds. The chrome rebuild time after touching base\win\registry.cc drops from ~200 seconds to less than 2 seconds, with 21 of 24 link steps skipped entirely. This also fixes some /INCREMENTAL /OPT:ICF mismatch warnings. BUG=608801,621236 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by brucedawson@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Enable incremental linking in release component builds Component builds are, by definition, not optimized for maximum performance. So incremental linking - which trades a little bit of performance for much faster build times - is appropriate in release component builds, as well as in debug builds. This change enables incremental linking in release component builds. This also requires turning off /OPT:ICF and /PROFILE in component builds because they are incompatible with /incremental, but they are performance related flags so they are also not important for component builds. blink_core.dll cannot be incrementally linked in debug x64 component builds, but it *can* be incrementally linked in release x64 component builds (and debug/release x86 component builds), and this change enables incremental linking of blink_core.dll in all supported configurations. This drops the chrome rebuild time after touching third_party\WebKit\Source\core\html\AutoplayExperimentHelper.cpp from ~260 seconds to 9 seconds. The chrome rebuild time after touching base\win\registry.cc drops from ~200 seconds to less than 2 seconds, with 21 of 24 link steps skipped entirely. This also fixes some /INCREMENTAL /OPT:ICF mismatch warnings. BUG=608801,621236 ========== to ========== Enable incremental linking in release component builds Component builds are, by definition, not optimized for maximum performance. So incremental linking - which trades a little bit of performance for much faster build times - is appropriate in release component builds, as well as in debug builds. This change enables incremental linking in release component builds. This also requires turning off /OPT:ICF and /PROFILE in component builds because they are incompatible with /incremental, but they are performance related flags so they are also not important for component builds. blink_core.dll cannot be incrementally linked in debug x64 component builds, but it *can* be incrementally linked in release x64 component builds (and debug/release x86 component builds), and this change enables incremental linking of blink_core.dll in all supported configurations. This drops the chrome rebuild time after touching third_party\WebKit\Source\core\html\AutoplayExperimentHelper.cpp from ~260 seconds to 9 seconds. The chrome rebuild time after touching base\win\registry.cc drops from ~200 seconds to less than 2 seconds, with 21 of 24 link steps skipped entirely. This also fixes some /INCREMENTAL /OPT:ICF mismatch warnings. BUG=608801,621236 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Enable incremental linking in release component builds Component builds are, by definition, not optimized for maximum performance. So incremental linking - which trades a little bit of performance for much faster build times - is appropriate in release component builds, as well as in debug builds. This change enables incremental linking in release component builds. This also requires turning off /OPT:ICF and /PROFILE in component builds because they are incompatible with /incremental, but they are performance related flags so they are also not important for component builds. blink_core.dll cannot be incrementally linked in debug x64 component builds, but it *can* be incrementally linked in release x64 component builds (and debug/release x86 component builds), and this change enables incremental linking of blink_core.dll in all supported configurations. This drops the chrome rebuild time after touching third_party\WebKit\Source\core\html\AutoplayExperimentHelper.cpp from ~260 seconds to 9 seconds. The chrome rebuild time after touching base\win\registry.cc drops from ~200 seconds to less than 2 seconds, with 21 of 24 link steps skipped entirely. This also fixes some /INCREMENTAL /OPT:ICF mismatch warnings. BUG=608801,621236 ========== to ========== Enable incremental linking in release component builds Component builds are, by definition, not optimized for maximum performance. So incremental linking - which trades a little bit of performance for much faster build times - is appropriate in release component builds, as well as in debug builds. This change enables incremental linking in release component builds. This also requires turning off /OPT:ICF and /PROFILE in component builds because they are incompatible with /incremental, but they are performance related flags so they are also not important for component builds. blink_core.dll cannot be incrementally linked in debug x64 component builds, but it *can* be incrementally linked in release x64 component builds (and debug/release x86 component builds), and this change enables incremental linking of blink_core.dll in all supported configurations. This drops the chrome rebuild time after touching third_party\WebKit\Source\core\html\AutoplayExperimentHelper.cpp from ~260 seconds to 9 seconds. The chrome rebuild time after touching base\win\registry.cc drops from ~200 seconds to less than 2 seconds, with 21 of 24 link steps skipped entirely. This also fixes some /INCREMENTAL /OPT:ICF mismatch warnings. BUG=608801,621236 Committed: https://crrev.com/7bf1119ce51d5e5b489c780bbacc332eabdd6f07 Cr-Commit-Position: refs/heads/master@{#418660} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7bf1119ce51d5e5b489c780bbacc332eabdd6f07 Cr-Commit-Position: refs/heads/master@{#418660} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
