Issue metadata
Sign in to add a comment
|
2.1% regression in system_health.memory_mobile at 492518:492573 |
||||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Aug 11 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8971563507272560336
,
Aug 11 2017
=== Auto-CCing suspected CL author dgozman@chromium.org === Hi dgozman@chromium.org, the bisect results pointed to your CL, please take a look at the results. === BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : Dmitry Gozman Commit : 17ffda1a19434a08a8ec98c947b60c2bb90046e1 Date : Tue Aug 08 05:20:56 2017 Subject: [DevTools] Force TouchEventFeatureDetection when enabling touch Bisect Details Configuration: android_nexus7_perf_bisect Benchmark : system_health.memory_mobile Metric : memory:chrome:renderer_processes:reported_by_chrome:v8:heap:allocated_objects_size_avg/browse_media/browse_media_youtube Change : 1.03% | 14870600.6667 -> 15023488.6667 Revision Result N chromium@492517 14870601 +- 37755.3 6 good chromium@492531 14863540 +- 86696.2 6 good chromium@492538 14864329 +- 46461.8 6 good chromium@492539 15034136 +- 44058.6 6 bad <-- chromium@492540 15013945 +- 51019.6 6 bad chromium@492542 15052189 +- 32340.3 6 bad chromium@492545 15054215 +- 41325.1 6 bad chromium@492573 15023489 +- 59473.5 6 bad Please refer to the following doc on diagnosing memory regressions: https://chromium.googlesource.com/chromium/src/+/master/docs/memory-infra/memory_benchmarks.md To Run This Test src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --pageset-repeat=1 --also-run-disabled-tests --story-filter=browse.media.youtube system_health.memory_mobile More information on addressing performance regressions: http://g.co/ChromePerformanceRegressions Debug information about this bisect: https://chromeperf.appspot.com/buildbucket_job_status/8971563507272560336 For feedback, file a bug with component Speed>Bisection
,
Aug 11 2017
Issue 754623 has been merged into this issue.
,
Aug 11 2017
Adding iclelland@ and bindings team. Any ideas how adding a conditional feature can affect memory_mobile? The rest of the change were DevTools changes which do not run in benchmarks.
,
Aug 15 2017
Unless adding the conditional feature is now instantiating a V8 object in every execution context which wasn't being instantiated before -- it can do this if it unconditionally calls ConstructorForType or ConstructorForTypeSlowCase -- there shouldn't be any effect like that. Now, that did happen before with origin trials -- see crbug.com/637148 -- but we changed all of the trial installation code paths at the time to avoid instantiating any unneeded objects. This CL did add a new path to install a trial, so that would be the first place I'd look.
,
Aug 15 2017
Thank you for explanation. The origin trial is only installed when someone emulates touch through DevTools, which should not be the case here at all. So the problem (if any) must be in common codepath. It looks like ConstructorForType is only called from the generated installTouchEventFeatureDetection with two parameters, and I don't see any invocations of that. Also, this seems to only affect Android, which should already have touch feature detection enabled through RuntimeEnabledFeature. Does that suggest you anything?
,
Aug 21 2017
Issue 754621 has been merged into this issue.
,
Aug 25 2017
If I had to guess, I'd imagine the modification to the IDL might be responsible. GlobalEventHandlers, for instance, is implemented by Window, Document and HTMLElement, and it wouldn't surprise me for a conditionally-enabled feature to be somewhat more memory-intensive than one that doesn't vary. In particular, IIUC unconditional features are baked into the prototype and instance templates, whereas conditional features actually have the functions installed on each prototype/instance object, which are at least 1 per context and in the case of instances may be more.
,
Aug 25 2017
Conditional features should always be installed on the prototype, *except* for the global object, in which case they should always be on the (singleton) instance. AFAIK, there should only be one binding per context.
,
Aug 25 2017
One per context is still more than one per process, which could conceivably contribute (though I still would not have expected a 2 MB impact).
,
Dec 5
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Aug 11 2017