Issue metadata
Sign in to add a comment
|
3.5% regression in thread_times.key_silk_cases at 399403:399406 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jun 13 2016
=== Auto-CCing suspected CL author brettw@chromium.org === Hi brettw@chromium.org, the bisect results pointed to your CL below as possibly causing a regression. Please have a look at this info and see whether your CL be related. ===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Default components to static libraries in GN build Author : brettw Commit description: Previously components would be source sets in non-component mode. Some test targets link large components in chrome but use almost none of them, and including all object files makes these links very slow. BUG= 617837 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2043873004 Cr-Commit-Position: refs/heads/master@{#399406} Commit : 3b678169f32f46b0bab203bfd0572fad1c314ab7 Date : Mon Jun 13 03:30:26 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@399402 3.33984 0.0101787 5 good chromium@399404 3.3238 0.0302879 5 good chromium@399405 3.34416 0.0192179 5 good chromium@399406 3.46871 0.00918205 5 bad <-- Bisect job ran on: android_nexus5_perf_bisect Bug ID: 619593 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests thread_times.key_silk_cases Test Metric: thread_renderer_compositor_cpu_time_per_frame/thread_renderer_compositor_cpu_time_per_frame Relative Change: 3.86% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5_perf_bisect/builds/3737 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9009958523835525680 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5271708617932800 | O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq | X | for more information addressing perf regression bugs. For feedback, | / \ | file a bug with component Tests>AutoBisect. Thank you!
,
Jun 13 2016
===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Default components to static libraries in GN build Author : brettw Commit description: Previously components would be source sets in non-component mode. Some test targets link large components in chrome but use almost none of them, and including all object files makes these links very slow. BUG= 617837 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2043873004 Cr-Commit-Position: refs/heads/master@{#399406} Commit : 3b678169f32f46b0bab203bfd0572fad1c314ab7 Date : Mon Jun 13 03:30:26 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@399402 3.32048 0.0120254 5 good chromium@399404 3.34565 0.0212969 5 good chromium@399405 3.32144 0.0184807 5 good chromium@399406 3.47838 0.0148741 5 bad <-- Bisect job ran on: android_nexus5_perf_bisect Bug ID: 619593 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests thread_times.key_silk_cases Test Metric: thread_renderer_compositor_cpu_time_per_frame/thread_renderer_compositor_cpu_time_per_frame Relative Change: 4.76% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5_perf_bisect/builds/3738 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9009958514124245824 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5776365798817792 | O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq | X | for more information addressing perf regression bugs. For feedback, | / \ | file a bug with component Tests>AutoBisect. Thank you!
,
Jun 24 2016
Brett: can you looks into whether your patch cause the regression in thread_times.key_silk_cases here?
,
Jul 1 2016
Brett, ping on this.
,
Jul 5 2016
Moving this nonessential bug to the next milestone. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 11 2016
brettw@ - Ping!
,
Jul 11 2016
This patch was reverted and relanded and it looks like this benchmark improved and then regressed again with those events, so it does seem like there's something related to my patch. In the attached image, the ! marks the initial landing, then it's reverted soon after, and is relanded right after the 399622 gridline. There's no clear reason why static libraries vs source sets would cause performance differences, since the same code should get getting linked in the end. We have seen instances where the linker duplicate-code-folds more aggressively using source sets, although have no explanation for this. It could be a related issue causing differences here. I can turn this behavior off for Android and we can check what the behavior is on this benchmark and build times (this patch was done to improve build times).
,
Jul 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/89248dc70996f9927292a7fee6911715464e1643 commit 89248dc70996f9927292a7fee6911715464e1643 Author: brettw <brettw@chromium.org> Date: Wed Jul 13 06:25:25 2016 Make components source_sets on Android. There was a perf regression reported when components were changed from source sets to static libraries in the non-component build. This patch switches back to source sets on Android only to see what happens to thread_times.key_silk_cases and the build performance. BUG= 619593 Review-Url: https://codereview.chromium.org/2137023003 Cr-Commit-Position: refs/heads/master@{#405056} [modify] https://crrev.com/89248dc70996f9927292a7fee6911715464e1643/build/config/BUILDCONFIG.gn
,
Jul 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aa5466df6e58dd54bc1dd1a5192f2c52ebf6ff29 commit aa5466df6e58dd54bc1dd1a5192f2c52ebf6ff29 Author: thakis <thakis@chromium.org> Date: Wed Jul 13 15:06:39 2016 Revert of Make components source_sets on Android. (patchset #1 id:1 of https://codereview.chromium.org/2137023003/ ) Reason for revert: Speculative, might've broke llvm_force_head_revision=true builds (see comment on the original review). Original issue's description: > Make components source_sets on Android. > > There was a perf regression reported when components were changed from source sets to static libraries in the non-component build. This patch switches back to source sets on Android only to see what happens to thread_times.key_silk_cases and the build performance. > > BUG= 619593 > > Committed: https://crrev.com/89248dc70996f9927292a7fee6911715464e1643 > Cr-Commit-Position: refs/heads/master@{#405056} TBR=dpranke@chromium.org,brettw@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 619593 Review-Url: https://codereview.chromium.org/2151523002 Cr-Commit-Position: refs/heads/master@{#405152} [modify] https://crrev.com/aa5466df6e58dd54bc1dd1a5192f2c52ebf6ff29/build/config/BUILDCONFIG.gn
,
Jul 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/edc7aa53ed38bbd5fbe330bab79f4ac9995321ae commit edc7aa53ed38bbd5fbe330bab79f4ac9995321ae Author: thakis <thakis@chromium.org> Date: Wed Jul 13 15:31:10 2016 Reland of Make components source_sets on Android. (patchset #1 id:1 of https://codereview.chromium.org/2151523002/ ) Reason for revert: Didn't help. Original issue's description: > Revert of Make components source_sets on Android. (patchset #1 id:1 of https://codereview.chromium.org/2137023003/ ) > > Reason for revert: > Speculative, might've broke llvm_force_head_revision=true builds (see comment on the original review). > > Original issue's description: > > Make components source_sets on Android. > > > > There was a perf regression reported when components were changed from source sets to static libraries in the non-component build. This patch switches back to source sets on Android only to see what happens to thread_times.key_silk_cases and the build performance. > > > > BUG= 619593 > > > > Committed: https://crrev.com/89248dc70996f9927292a7fee6911715464e1643 > > Cr-Commit-Position: refs/heads/master@{#405056} > > TBR=dpranke@chromium.org,brettw@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG= 619593 > > Committed: https://crrev.com/aa5466df6e58dd54bc1dd1a5192f2c52ebf6ff29 > Cr-Commit-Position: refs/heads/master@{#405152} TBR=dpranke@chromium.org,brettw@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 619593 Review-Url: https://codereview.chromium.org/2145063002 Cr-Commit-Position: refs/heads/master@{#405159} [modify] https://crrev.com/edc7aa53ed38bbd5fbe330bab79f4ac9995321ae/build/config/BUILDCONFIG.gn
,
Jul 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/89248dc70996f9927292a7fee6911715464e1643 commit 89248dc70996f9927292a7fee6911715464e1643 Author: brettw <brettw@chromium.org> Date: Wed Jul 13 06:25:25 2016 Make components source_sets on Android. There was a perf regression reported when components were changed from source sets to static libraries in the non-component build. This patch switches back to source sets on Android only to see what happens to thread_times.key_silk_cases and the build performance. BUG= 619593 Review-Url: https://codereview.chromium.org/2137023003 Cr-Commit-Position: refs/heads/master@{#405056} [modify] https://crrev.com/89248dc70996f9927292a7fee6911715464e1643/build/config/BUILDCONFIG.gn
,
Jul 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/aa5466df6e58dd54bc1dd1a5192f2c52ebf6ff29 commit aa5466df6e58dd54bc1dd1a5192f2c52ebf6ff29 Author: thakis <thakis@chromium.org> Date: Wed Jul 13 15:06:39 2016 Revert of Make components source_sets on Android. (patchset #1 id:1 of https://codereview.chromium.org/2137023003/ ) Reason for revert: Speculative, might've broke llvm_force_head_revision=true builds (see comment on the original review). Original issue's description: > Make components source_sets on Android. > > There was a perf regression reported when components were changed from source sets to static libraries in the non-component build. This patch switches back to source sets on Android only to see what happens to thread_times.key_silk_cases and the build performance. > > BUG= 619593 > > Committed: https://crrev.com/89248dc70996f9927292a7fee6911715464e1643 > Cr-Commit-Position: refs/heads/master@{#405056} TBR=dpranke@chromium.org,brettw@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 619593 Review-Url: https://codereview.chromium.org/2151523002 Cr-Commit-Position: refs/heads/master@{#405152} [modify] https://crrev.com/aa5466df6e58dd54bc1dd1a5192f2c52ebf6ff29/build/config/BUILDCONFIG.gn
,
Jul 13 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/edc7aa53ed38bbd5fbe330bab79f4ac9995321ae commit edc7aa53ed38bbd5fbe330bab79f4ac9995321ae Author: thakis <thakis@chromium.org> Date: Wed Jul 13 15:31:10 2016 Reland of Make components source_sets on Android. (patchset #1 id:1 of https://codereview.chromium.org/2151523002/ ) Reason for revert: Didn't help. Original issue's description: > Revert of Make components source_sets on Android. (patchset #1 id:1 of https://codereview.chromium.org/2137023003/ ) > > Reason for revert: > Speculative, might've broke llvm_force_head_revision=true builds (see comment on the original review). > > Original issue's description: > > Make components source_sets on Android. > > > > There was a perf regression reported when components were changed from source sets to static libraries in the non-component build. This patch switches back to source sets on Android only to see what happens to thread_times.key_silk_cases and the build performance. > > > > BUG= 619593 > > > > Committed: https://crrev.com/89248dc70996f9927292a7fee6911715464e1643 > > Cr-Commit-Position: refs/heads/master@{#405056} > > TBR=dpranke@chromium.org,brettw@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG= 619593 > > Committed: https://crrev.com/aa5466df6e58dd54bc1dd1a5192f2c52ebf6ff29 > Cr-Commit-Position: refs/heads/master@{#405152} TBR=dpranke@chromium.org,brettw@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 619593 Review-Url: https://codereview.chromium.org/2145063002 Cr-Commit-Position: refs/heads/master@{#405159} [modify] https://crrev.com/edc7aa53ed38bbd5fbe330bab79f4ac9995321ae/build/config/BUILDCONFIG.gn
,
Jul 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/322360f3cd7849ab6fd349f34c0f3709a0294b72 commit 322360f3cd7849ab6fd349f34c0f3709a0294b72 Author: brettw <brettw@chromium.org> Date: Thu Jul 14 19:42:14 2016 Update build comment with results of analysis. This is related to a comment about components being source sets on Android. A previous patch speculatively changed a build parameter with a comment referencing the speculative test. Now we know this does affect the runtime but not build performance, so we will keep the change and the comment needs to be updated to reflect this. BUG= 619593 Review-Url: https://codereview.chromium.org/2152743003 Cr-Commit-Position: refs/heads/master@{#405546} [modify] https://crrev.com/322360f3cd7849ab6fd349f34c0f3709a0294b72/build/config/BUILDCONFIG.gn
,
Jul 14 2016
The regression is now fixed because my change no longer applies to Android. But why this affects performance is still a mystery.
,
May 22 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0ffeb3f08464644582f40e9e7c41ffdfab00a836 commit 0ffeb3f08464644582f40e9e7c41ffdfab00a836 Author: Nico Weber <thakis@chromium.org> Date: Tue May 22 16:45:06 2018 Make components behave the same on android and elsewhere. Maybe this no longer negatively impacts performance now, and it's simpler. Bug: 619593 Change-Id: If7ce241644093d2a918ea9c525bf848ac59661c5 Reviewed-on: https://chromium-review.googlesource.com/1024823 Reviewed-by: Dirk Pranke <dpranke@chromium.org> Commit-Queue: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#560648} [modify] https://crrev.com/0ffeb3f08464644582f40e9e7c41ffdfab00a836/build/config/BUILDCONFIG.gn |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by oth@chromium.org
, Jun 13 2016