Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2078223002:
Add a dedicated "optimize_speed" config to GN. (Closed)
|
Created:
4 years, 6 months ago by Dirk Pranke Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a dedicated "optimize_speed" config to GN.
The various GN configs to select different optimization levels are
confusing, and sorting them out will take a decent amount of
perf testing, but in order to achieve parity w/ GYP, for now
we need a dedicated config to make sure some components (e.g., v8)
are compiled w/ -O3 where appropriate.
R=brettw@chromium.org, machenbach@chromium.org
BUG=616031, 618678, 621335
Committed: https://crrev.com/c266bec7641b1926f65d1f48614aafb2008a7c57
Cr-Commit-Position: refs/heads/master@{#400828}
Patch Set 1 #
Total comments: 2
Patch Set 2 : update w/ review feedback #Patch Set 3 : update w/ review feedback, call it "optimize_speed" instead and make it cross-platform #
Total comments: 2
Messages
Total messages: 20 (7 generated)
See https://codereview.chromium.org/2076403002 for the intended usage in v8. I don't really like this solution, but "optimize_max" is used all over the place in the code and I think reworking it to be consistent without introducing improper size/speed tradeoffs on the different platforms is something we can't really afford to do before the M53 branch.
https://codereview.chromium.org/2078223002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2078223002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1378: ldflags = common_optimize_on_ldflags Don't we miss the common_optimize_on_ldflags if we just use the new optimize_posix_target_for_speed below?
ALso: You called it optimize_posix_target_for_speed in the code. Please update either code or CL desc.
Will standardize on 'optimize_posix_target_for_speed'. https://codereview.chromium.org/2078223002/diff/1/build/config/compiler/BUILD.gn File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2078223002/diff/1/build/config/compiler/BUILD... build/config/compiler/BUILD.gn:1378: ldflags = common_optimize_on_ldflags On 2016/06/20 11:38:25, Michael Achenbach wrote: > Don't we miss the common_optimize_on_ldflags if we just use the new > optimize_posix_target_for_speed below? Whoops, yes. Good catch. Will fix.
Description was changed from ========== Add a dedicated "optimize_target_for_speed" config to GN. The various GN configs to select different optimization levels are confusing, and sorting them out will take a decent amount of perf testing, but in order to achieve parity w/ GYP, for now we need a dedicated config to make sure some components (e.g., v8) are compiled w/ -O3 where appropriate. R=brettw@chromium.org, machenbach@chromium.org BUG=616031, 618678, 621335 ========== to ========== Add a dedicated "optimize_posix_target_for_speed" config to GN. The various GN configs to select different optimization levels are confusing, and sorting them out will take a decent amount of perf testing, but in order to achieve parity w/ GYP, for now we need a dedicated config to make sure some components (e.g., v8) are compiled w/ -O3 where appropriate. R=brettw@chromium.org, machenbach@chromium.org BUG=616031, 618678, 621335 ==========
Updated. Please take another look?
lgtm
Description was changed from ========== Add a dedicated "optimize_posix_target_for_speed" config to GN. The various GN configs to select different optimization levels are confusing, and sorting them out will take a decent amount of perf testing, but in order to achieve parity w/ GYP, for now we need a dedicated config to make sure some components (e.g., v8) are compiled w/ -O3 where appropriate. R=brettw@chromium.org, machenbach@chromium.org BUG=616031, 618678, 621335 ========== to ========== Add a dedicated "optimize_speed" config to GN. The various GN configs to select different optimization levels are confusing, and sorting them out will take a decent amount of perf testing, but in order to achieve parity w/ GYP, for now we need a dedicated config to make sure some components (e.g., v8) are compiled w/ -O3 where appropriate. R=brettw@chromium.org, machenbach@chromium.org BUG=616031, 618678, 621335 ==========
lgtm
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from machenbach@chromium.org Link to the patchset: https://codereview.chromium.org/2078223002/#ps40001 (title: "update w/ review feedback, call it "optimize_speed" instead and make it cross-platform")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078223002/40001
Message was sent while issue was closed.
Description was changed from ========== Add a dedicated "optimize_speed" config to GN. The various GN configs to select different optimization levels are confusing, and sorting them out will take a decent amount of perf testing, but in order to achieve parity w/ GYP, for now we need a dedicated config to make sure some components (e.g., v8) are compiled w/ -O3 where appropriate. R=brettw@chromium.org, machenbach@chromium.org BUG=616031, 618678, 621335 ========== to ========== Add a dedicated "optimize_speed" config to GN. The various GN configs to select different optimization levels are confusing, and sorting them out will take a decent amount of perf testing, but in order to achieve parity w/ GYP, for now we need a dedicated config to make sure some components (e.g., v8) are compiled w/ -O3 where appropriate. R=brettw@chromium.org, machenbach@chromium.org BUG=616031, 618678, 621335 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add a dedicated "optimize_speed" config to GN. The various GN configs to select different optimization levels are confusing, and sorting them out will take a decent amount of perf testing, but in order to achieve parity w/ GYP, for now we need a dedicated config to make sure some components (e.g., v8) are compiled w/ -O3 where appropriate. R=brettw@chromium.org, machenbach@chromium.org BUG=616031, 618678, 621335 ========== to ========== Add a dedicated "optimize_speed" config to GN. The various GN configs to select different optimization levels are confusing, and sorting them out will take a decent amount of perf testing, but in order to achieve parity w/ GYP, for now we need a dedicated config to make sure some components (e.g., v8) are compiled w/ -O3 where appropriate. R=brettw@chromium.org, machenbach@chromium.org BUG=616031, 618678, 621335 Committed: https://crrev.com/c266bec7641b1926f65d1f48614aafb2008a7c57 Cr-Commit-Position: refs/heads/master@{#400828} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c266bec7641b1926f65d1f48614aafb2008a7c57 Cr-Commit-Position: refs/heads/master@{#400828}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2078223002/diff/40001/build/config/compiler/B... File build/config/compiler/BUILD.gn (right): https://codereview.chromium.org/2078223002/diff/40001/build/config/compiler/B... build/config/compiler/BUILD.gn:1409: config("optimize_speed") { bikeshed: I think it'd be good if the name contained "also, this will increase binary size" – else people will see this and think "well speed is good i better add this to my target!" https://codereview.chromium.org/2078223002/diff/40001/build/config/compiler/B... build/config/compiler/BUILD.gn:1437: ] this is a lot of copy-pasta :-/
Message was sent while issue was closed.
Yes, everybody agrees these need to be refactored. Dirk and I had a long discussion about naming.
Message was sent while issue was closed.
":optimize_fast_and_bloated" |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
