New issue
Advanced search Search tips

Issue 754636 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Dec 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

2.1% regression in system_health.memory_mobile at 492518:492573

Project Member Reported by jgruber@chromium.org, Aug 11 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Aug 11 2017

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=754636

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=0d8e00a0c3cd1af7083665f249d1643c09d62575d5a60ffbaba7fc4cd194670c


Bot(s) for this bug's original alert(s):

android-nexus7v2
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Aug 11 2017

Cc: dgozman@chromium.org
Owner: dgozman@chromium.org
Status: Assigned (was: Untriaged)

=== 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
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Aug 11 2017

 Issue 754623  has been merged into this issue.
Cc: -dgozman@chromium.org iclell...@chromium.org
Components: Blink>Bindings
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.
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.
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?

Project Member

Comment 8 by 42576172...@developer.gserviceaccount.com, Aug 21 2017

Issue 754621 has been merged into this issue.
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.
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.
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).
Status: WontFix (was: Assigned)

Sign in to add a comment