Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2270693006:
Adjust skia build settings to match gyp, improve perf (Closed)
|
Created:
4 years, 4 months ago by brucedawson Modified:
4 years, 3 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. |
DescriptionAdjust skia build settings to match gyp, improve perf
When official builds switched from gyp to gn some smoothness tests
regressed. Investigation showed that some skia functions were taking
up to 15x longer to execute. This was due to /GL /O2 being changed to
/O1 (LTCG and optimize-for-speed changing to optimize-for-size). This
change resolves most of the compiler option differences in skia and
dramatically reduces the CPU usage in the gn build, from 9.4 s to 5.3,
within spitting distance of the gyp build.
This increases the size of chrome_child.dll from 48,593,408 to
49,213,440 bytes. The gyp version is 49,032,192 bytes and if necessary
some more size optimizations can be applied to the gn build.
The discrepancy between gyp and gn optimization settings in skia occurred
when crrev.com/1410883008 landed for gyp only.
There may be other differences - investigation continues - but this fixes
the main issues noticed.
BUG=632651
Committed: https://crrev.com/3e80255c5139698d5fca9d8f1595a307f6cb3639
Cr-Commit-Position: refs/heads/master@{#414368}
Patch Set 1 #Patch Set 2 : Merge the is_android and is_win conditions #Patch Set 3 : Another place to add optimize_max to Windows as well #Messages
Total messages: 28 (16 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...
brucedawson@chromium.org changed reviewers: + dpranke@chromium.org
This fix works, but I'm not sure if it is the correct place, or what our overall optimization settings policy is. I also need to check for other discrepancies, although there are probably none as critical as this. For details see the bug, especially https://bugs.chromium.org/p/chromium/issues/detail?id=632651#c17, and the two before it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dpranke@chromium.org changed reviewers: + borenet@chromium.org
I wonder if this should just be optimize_max across the board, or at least in non-debug builds? @borenet - anyone in skia that might have an informed opinion here?
borenet@google.com changed reviewers: + borenet@google.com, jcgregorio@chromium.org, mtklein@chromium.org, reed@chromium.org
+others
lgtm
On 2016/08/24 18:32:04, mtklein_C wrote: > lgtm I changed it so that the win and android builds share the changes, which also fixes the error in my original change that had it applying optimize_max in debug builds. But it does look weird having optimize_max just for win and android. In src\skia\skia.gyp it looks like 'optimize': 'max' is selected everywhere, for all platforms. I did find crbug.com/543583 which says that with VC++ 2013 it was actually *required* to have optimize_max on for official builds, to avoid compiler bugs.
Description was changed from ========== Adjust skia build settings to match gyp, improve perf When official builds switched from gyp to gn some smoothness tests regressed. Investigation showed that some skia functions were taking up to 15x longer to execute. This was due to /GL /O2 being changed to /O1 (LTCG and optimize-for-speed changing to optimize-for-size). This change resolves most of the compiler option differences in skia and dramatically reduces the CPU usage in the gn build, from 9.4 s to 5.3, within spitting distance of the gyp build. This increases the size of chrome_child.dll from 48,593,408 to 49,213,440 bytes. The gyp version is 49,032,192 bytes and if necessary some more size optimizations can be applied to the gn build. There may be other differences - investigation continues. BUG=632651 ========== to ========== Adjust skia build settings to match gyp, improve perf When official builds switched from gyp to gn some smoothness tests regressed. Investigation showed that some skia functions were taking up to 15x longer to execute. This was due to /GL /O2 being changed to /O1 (LTCG and optimize-for-speed changing to optimize-for-size). This change resolves most of the compiler option differences in skia and dramatically reduces the CPU usage in the gn build, from 9.4 s to 5.3, within spitting distance of the gyp build. This increases the size of chrome_child.dll from 48,593,408 to 49,213,440 bytes. The gyp version is 49,032,192 bytes and if necessary some more size optimizations can be applied to the gn build. The discrepancy between gyp and gn optimization settings in skia occurred when crrev.com/1410883008 landed for gyp only. There may be other differences - investigation continues - but this fixes the main issues noticed. BUG=632651 ==========
The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run
I've thought about this some more and, assuming that the try bots are happy, I think I should land this as is. But, I could be convinced to remove the platform checks completely, which would enable optimizations for OSX, Linux, etc. as well. Any thoughts? I'm still trying to track down what may be a remaining regression but that could take a while and may not even exist, so I don't want to wait on it.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I think land as-is and change for the other platforms in a follow-up is probably the thing to try.
I think land as-is and change for the other platforms in a follow-up is probably the thing to try.
On 2016/08/24 at 21:54:07, brucedawson wrote: > On 2016/08/24 18:32:04, mtklein_C wrote: > > lgtm > > I changed it so that the win and android builds share the changes, which also fixes the error in my original change that had it applying optimize_max in debug builds. > > But it does look weird having optimize_max just for win and android. In src\skia\skia.gyp it looks like 'optimize': 'max' is selected everywhere, for all platforms. > > I did find crbug.com/543583 which says that with VC++ 2013 it was actually *required* to have optimize_max on for official builds, to avoid compiler bugs. Yep, if I remember correctly, there was a function call (might have been __vectorcall) to some code implemening blend modes that was being set up incorrectly. The caller didn't fill the arguments into the right registers (I vaugely remember these been xmm0 and xmm1, which I why I keep thinking it was __vectorcall) and that caused the callee to just draw all kinds of wrong. I think the speculation was that the problem was spanning calls across compilation units compiled with different optimization levels, so the simplest thing was to just crank all of Skia up to the max. Must have been the callee was already at max optimization, and the caller not? I don't think we ever tested that VS 2015 fixed any of that. I don't think anyone working on Skia minds too much which flags Chrome chooses to build Skia with. Certainly no one would anyone complain if you want to follow up by setting all platforms to max optimization. There's plenty of code which will run faster with -O3 than -Os, but probably not so much faster that it's an urgent need. Usually if something's that performance critical we try to take over manually with intrinsics. But I wouldn't be surprised if we find some code paths that have just been waiting for us to turn on a more aggressive autovectorizer or some more aggressive control flow rearrangement.
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
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@chromium.org Link to the patchset: https://codereview.chromium.org/2270693006/#ps40001 (title: "Another place to add optimize_max to Windows as well")
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 ========== Adjust skia build settings to match gyp, improve perf When official builds switched from gyp to gn some smoothness tests regressed. Investigation showed that some skia functions were taking up to 15x longer to execute. This was due to /GL /O2 being changed to /O1 (LTCG and optimize-for-speed changing to optimize-for-size). This change resolves most of the compiler option differences in skia and dramatically reduces the CPU usage in the gn build, from 9.4 s to 5.3, within spitting distance of the gyp build. This increases the size of chrome_child.dll from 48,593,408 to 49,213,440 bytes. The gyp version is 49,032,192 bytes and if necessary some more size optimizations can be applied to the gn build. The discrepancy between gyp and gn optimization settings in skia occurred when crrev.com/1410883008 landed for gyp only. There may be other differences - investigation continues - but this fixes the main issues noticed. BUG=632651 ========== to ========== Adjust skia build settings to match gyp, improve perf When official builds switched from gyp to gn some smoothness tests regressed. Investigation showed that some skia functions were taking up to 15x longer to execute. This was due to /GL /O2 being changed to /O1 (LTCG and optimize-for-speed changing to optimize-for-size). This change resolves most of the compiler option differences in skia and dramatically reduces the CPU usage in the gn build, from 9.4 s to 5.3, within spitting distance of the gyp build. This increases the size of chrome_child.dll from 48,593,408 to 49,213,440 bytes. The gyp version is 49,032,192 bytes and if necessary some more size optimizations can be applied to the gn build. The discrepancy between gyp and gn optimization settings in skia occurred when crrev.com/1410883008 landed for gyp only. There may be other differences - investigation continues - but this fixes the main issues noticed. BUG=632651 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Adjust skia build settings to match gyp, improve perf When official builds switched from gyp to gn some smoothness tests regressed. Investigation showed that some skia functions were taking up to 15x longer to execute. This was due to /GL /O2 being changed to /O1 (LTCG and optimize-for-speed changing to optimize-for-size). This change resolves most of the compiler option differences in skia and dramatically reduces the CPU usage in the gn build, from 9.4 s to 5.3, within spitting distance of the gyp build. This increases the size of chrome_child.dll from 48,593,408 to 49,213,440 bytes. The gyp version is 49,032,192 bytes and if necessary some more size optimizations can be applied to the gn build. The discrepancy between gyp and gn optimization settings in skia occurred when crrev.com/1410883008 landed for gyp only. There may be other differences - investigation continues - but this fixes the main issues noticed. BUG=632651 ========== to ========== Adjust skia build settings to match gyp, improve perf When official builds switched from gyp to gn some smoothness tests regressed. Investigation showed that some skia functions were taking up to 15x longer to execute. This was due to /GL /O2 being changed to /O1 (LTCG and optimize-for-speed changing to optimize-for-size). This change resolves most of the compiler option differences in skia and dramatically reduces the CPU usage in the gn build, from 9.4 s to 5.3, within spitting distance of the gyp build. This increases the size of chrome_child.dll from 48,593,408 to 49,213,440 bytes. The gyp version is 49,032,192 bytes and if necessary some more size optimizations can be applied to the gn build. The discrepancy between gyp and gn optimization settings in skia occurred when crrev.com/1410883008 landed for gyp only. There may be other differences - investigation continues - but this fixes the main issues noticed. BUG=632651 Committed: https://crrev.com/3e80255c5139698d5fca9d8f1595a307f6cb3639 Cr-Commit-Position: refs/heads/master@{#414368} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3e80255c5139698d5fca9d8f1595a307f6cb3639 Cr-Commit-Position: refs/heads/master@{#414368} |
