Issue metadata
Sign in to add a comment
|
81.5%-112.7% regression in blink_perf.pywebsocket at 384059:384117 |
||||||||||||||||||||||
Issue descriptionLooking at the commit range [1] the only obvious suspect is crrev.com/384093: "[BlobAsync] Asynchronous Blob Construction Final Patch" by dmurph@. [1]https://chromium.googlesource.com/chromium/src/+log/71305613fab6397a9c65f7af7f6154039ebe5df7..387ffbcef8667b2c04ef00e48de49a777ece556d
,
Mar 31 2016
,
Mar 31 2016
Kicked off a bisect: https://chromeperf.appspot.com/buildbucket_job_status/9016662404170099952
,
Mar 31 2016
=== Auto-CCing suspected CL author dmurph@chromium.org === Hi dmurph@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 : [BlobAsync] Asynchronous Blob Construction Final Patch Author : dmurph Commit description: This is the final patch that hooks up the blob storage system to the new async protocol. In this patch we: * Hook up the new classes and modules we created in the previous patches. * Remove the old way of creating blobs. * Create a few classes like blob_message_filter and blob_dispatcher_host for handing IPC messages. The result of this change makes renderer-initiated blob construction asynchronous instead of synchronous. So constructing new blobs should be faster, but the time from construction to read should be the same, as the reading still has to wait for the blob to be transferred to the browser. Because we let the renderer continue before we've sent all of the data, we use ChildProcess::AddRefProcess() system to keep the renderer alive while we transfer data. Patches: 1: https://codereview.chromium.org/1287303002 (committed!) 2: https://codereview.chromium.org/1288373002 (committed!) 3: https://codereview.chromium.org/1292523002 (committed!) 4: https://codereview.chromium.org/1098853003 (committed!) Hookup: https://codereview.chromium.org/1234813004 BUG= 375297 Review URL: https://codereview.chromium.org/1234813004 Cr-Commit-Position: refs/heads/master@{#384093} Commit : 1fb98480c61c563587cbb01de87cfad180fec942 Date : Wed Mar 30 21:15:51 2016 ===== TESTED REVISIONS ===== Revision Mean Value Std. Dev. Num Values Good? chromium@384090 8.8 1.012423 5 good chromium@384092 8.82 0.443847 5 good chromium@384093 16.86 0.719722 5 bad <- chromium@384095 16.96 0.350714 5 bad chromium@384100 17.18 0.779102 5 bad Bisect job ran on: linux_perf_bisect Bug ID: 599490 Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --also-run-disabled-tests blink_perf.pywebsocket Test Metric: fetch-send-blob-window-async-verify/fetch-send-blob-window-async-verify Relative Change: 95.23% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bisect/builds/6362 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9016662404170099952 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=599490 | 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!
,
Mar 31 2016
===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : [BlobAsync] Asynchronous Blob Construction Final Patch Author : dmurph Commit description: This is the final patch that hooks up the blob storage system to the new async protocol. In this patch we: * Hook up the new classes and modules we created in the previous patches. * Remove the old way of creating blobs. * Create a few classes like blob_message_filter and blob_dispatcher_host for handing IPC messages. The result of this change makes renderer-initiated blob construction asynchronous instead of synchronous. So constructing new blobs should be faster, but the time from construction to read should be the same, as the reading still has to wait for the blob to be transferred to the browser. Because we let the renderer continue before we've sent all of the data, we use ChildProcess::AddRefProcess() system to keep the renderer alive while we transfer data. Patches: 1: https://codereview.chromium.org/1287303002 (committed!) 2: https://codereview.chromium.org/1288373002 (committed!) 3: https://codereview.chromium.org/1292523002 (committed!) 4: https://codereview.chromium.org/1098853003 (committed!) Hookup: https://codereview.chromium.org/1234813004 BUG= 375297 Review URL: https://codereview.chromium.org/1234813004 Cr-Commit-Position: refs/heads/master@{#384093} Commit : 1fb98480c61c563587cbb01de87cfad180fec942 Date : Wed Mar 30 21:15:51 2016 ===== TESTED REVISIONS ===== Revision Mean Value Std. Dev. Num Values Good? chromium@384090 8.16 0.343511 5 good chromium@384092 8.06 0.181659 5 good chromium@384093 18.08 0.605805 5 bad <- chromium@384095 17.6 0.620484 5 bad chromium@384100 17.3 0.353553 5 bad Bisect job ran on: linux_perf_bisect Bug ID: 599490 Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --also-run-disabled-tests blink_perf.pywebsocket Test Metric: fetch-send-blob-window-async-verify/fetch-send-blob-window-async-verify Relative Change: 112.01% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/linux_perf_bisect/builds/6363 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9016661400961674752 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=599490 | 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!
,
Mar 31 2016
Hmm..... So the create-and-immediately-read functionality of blobs should get a little slower, but this seems like a lot? I'm basically scheduling one more event loop task in-between the blob data being sent and the read. Maybe I should look into possibly shortcutting this.
,
Mar 31 2016
Hey michael, what if we allowed the webblobregistry_impl to send the StartBuildingBlob message if the total size is < IPC size? this should speed up the small blob path, and hopefully remove this regression.
,
Apr 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/90774e39bb24b1c531e9b3aba16c77ab96675fb3 commit 90774e39bb24b1c531e9b3aba16c77ab96675fb3 Author: dmurph <dmurph@chromium.org> Date: Wed Apr 06 16:20:43 2016 [BlobAsync] Faster shortcuttin, make renderer controller leaky & alive. This should fix strange memory access crashes observed in the following bugs by doing the following things: 1. We make the controller a leaky lazy instance so it isn't destructed. 2. We make one less message loop hop before we call AddProcessRef, so hopefully our process won't be shutting down in the middle of a transfer. 3. We send the 'descriptions' as early as possible if we're below the IPC threshold, which should help some of the speed regressions. 4. We prevent sudden shutdowns (like tab bar closing) by using blink::Platform::current()->suddenTerminationChanged(false); BUG=600462,600435, 599490 , 599416 , 375297 Review URL: https://codereview.chromium.org/1853333003 Cr-Commit-Position: refs/heads/master@{#385472} [modify] https://crrev.com/90774e39bb24b1c531e9b3aba16c77ab96675fb3/content/browser/blob_storage/blob_async_builder_host_unittest.cc [modify] https://crrev.com/90774e39bb24b1c531e9b3aba16c77ab96675fb3/content/browser/blob_storage/blob_dispatcher_host.cc [modify] https://crrev.com/90774e39bb24b1c531e9b3aba16c77ab96675fb3/content/browser/blob_storage/blob_dispatcher_host.h [modify] https://crrev.com/90774e39bb24b1c531e9b3aba16c77ab96675fb3/content/browser/blob_storage/blob_dispatcher_host_unittest.cc [modify] https://crrev.com/90774e39bb24b1c531e9b3aba16c77ab96675fb3/content/child/blob_storage/blob_transport_controller.cc [modify] https://crrev.com/90774e39bb24b1c531e9b3aba16c77ab96675fb3/content/child/blob_storage/blob_transport_controller.h [modify] https://crrev.com/90774e39bb24b1c531e9b3aba16c77ab96675fb3/content/child/blob_storage/blob_transport_controller_unittest.cc [modify] https://crrev.com/90774e39bb24b1c531e9b3aba16c77ab96675fb3/content/child/webblobregistry_impl.cc [modify] https://crrev.com/90774e39bb24b1c531e9b3aba16c77ab96675fb3/content/child/webblobregistry_impl.h [modify] https://crrev.com/90774e39bb24b1c531e9b3aba16c77ab96675fb3/storage/browser/blob/blob_async_builder_host.cc [modify] https://crrev.com/90774e39bb24b1c531e9b3aba16c77ab96675fb3/storage/browser/blob/blob_async_builder_host.h
,
Apr 11 2016
,
Apr 14 2016
Sorry for confusing name, but the test is checking XHR's performance by using pywebsocket as a benchmark helper. https://chromeperf.appspot.com/report?sid=2e583f5c7f73651253b16d180bf0f9ba5d5c26ff6eeb549ff39450157e5361cf&rev=384100
,
Apr 14 2016
Yes, and it's using blobs to do this (writing and reading them). The regression is purely blob related, and doesn't effect the XHR code. So I'm guessing this doesn't matter for this test.
,
Apr 20 2016
Making as wontfix unless there are other concerns. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by majidvp@chromium.org
, Mar 31 2016