New issue
Advanced search Search tips

Issue 818672 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

9.5%-11.6% regression in system_health.memory_desktop at 540483:540609

Project Member Reported by rmcilroy@chromium.org, Mar 5 2018

Issue description

See the link to graphs below.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=818672

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


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

chromium-rel-mac11-air
chromium-rel-mac11-pro
Cc: primiano@chromium.org erikc...@chromium.org
Owner: erikc...@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/11818254440000

Stop emitting Linux-specific VMRegion stats on non-Linux derived OSes. by erikchen@chromium.org
https://chromium.googlesource.com/chromium/src/+/a1b73669d1e45b07a2d7a4cf0cfd5dbfe08d2a6d

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
Cc: ssid@chromium.org
The code in question removes some memory-related metrics emission from macOS, so it's surprising that it causes a ~6MB regression.

I suspect we're observing a problem whereby the timing of the malloc MDP emission has changed, thus changing the results. 

I think we really want to isolate the timing of the malloc MDP [to run first, before all other MDPs], which should help reduce noise a lot

primiano, ssid - thoughts?

Comment 5 by ssid@chromium.org, Mar 20 2018

Owner: rnep...@chromium.org
No this is not the issue with timiing of dump provider. this is the issue with when the dump was triggered.
The interval is set to 1 second and the request is fired every 1 second artificially by telemetry. The right thing to do here is to use the periodic dumps in memory dump config to get dumps every 1 second.

The difference in the time keeps increasing over time because the dump time has decreased, and the devtool inspector DumpMemory returns only after the dump is complete. Memory infra handles this case by request dumps exactly every 1 second instead of counting time after dump is finished.

Assigning this to rnephew@ who wrote this code.
The code should set dump interval using ChromeTraceConfig's memory_dump_config.AddTrigger()

Comment 6 by ssid@chromium.org, Mar 20 2018

Cc: perezju@chromium.org
+perezju in case the trace config mentioned in #5 was deprecated for some reason.
Owner: ----
Status: Untriaged (was: Assigned)
-rnephew, no longer working in this area

The theory in #5 works well for long_running:tools:gmail-background, you can file a bug on me to get that fixed and try to use periodic dumping from memory-infra and not from Telemetry.

Note however on all other stories we only ever request a single dump from Telemetry, so those regressions remain unexplained.

I'll kick off another pinpoint job on the 800 KiB regression in load:media:youtube to double check if they have the same root cause.
Project Member

Comment 9 by 42576172...@developer.gserviceaccount.com, Mar 21 2018

Cc: ishell@chromium.org adamk@chromium.org cbruni@chromium.org gsat...@chromium.org
Owner: cbruni@chromium.org
Status: Assigned (was: Untriaged)
📍 Found significant differences after each of 3 commits.
https://pinpoint-dot-chromeperf.appspot.com/job/14bb9e95440000

[runtime] Move validity cell from PrototypeInfo to Map. by ishell@chromium.org
https://chromium.googlesource.com/v8/v8/+/40a3e6dcb9bdb5cb2a7f6e29fa8e4de75e538ef6

Reland "[parser] Remove pretenuring of closures assigned to properties" by adamk@chromium.org
https://chromium.googlesource.com/v8/v8/+/3d7ad2e7e53d3cd6add51512580c054abf7e5805

[runtime] Always store the name in a function's ScopeInfo by cbruni@chromium.org
https://chromium.googlesource.com/v8/v8/+/01488b9c4f1b4c8e2b66494b24d7e7ff8a826860

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
Owner: adamk@chromium.org
My CL will cause a temporary regression since the name is field is duplicated between the SFI an the associated ScopeInfo. Future CL should reduce this overhead.

Biggest regression seems to be the parser reland.

Comment 11 by adamk@chromium.org, Mar 22 2018

My plan is to revert this attempt, we've hit too many regressions over the last weeks since this landed.
Project Member

Comment 12 by bugdroid1@chromium.org, Mar 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/5cf2ae5a4d7224198a0474eb27cc618b6843bc32

commit 5cf2ae5a4d7224198a0474eb27cc618b6843bc32
Author: Adam Klein <adamk@chromium.org>
Date: Mon Mar 26 19:09:22 2018

Revert "Reland "[parser] Remove pretenuring of closures assigned to properties""

This reverts commit 3d7ad2e7e53d3cd6add51512580c054abf7e5805.

Reason for revert: too many regressions to handle for now.

Original change's description:
> Reland "[parser] Remove pretenuring of closures assigned to properties"
>
> The memory gains were significant, so despite the bluebird-doxbee
> regression, we think it's better to have this patch than not.
> See the attached Chromium bug for more discussion.
>
> This is a reland of 20e346bd08591caff2f7c45de762ca4d4e3f72c8.
>
> Original change's description:
> > [parser] Remove pretenuring of closures assigned to properties
> >
> > This pretenuring was added in https://codereview.chromium.org/5220007,
> > back when it was necessary in order to allow use of the closure
> > as a "constant function" property. This should no longer be the case,
> > and the pretenuring causes some unfortunate downstream effects.
> >
> > This patch removes the parser's setting of this bit. If it doesn't
> > cause regressions on the perf bots, followup CLs will remove the
> > rest of the support for this feature.
> >
> > Bug:  v8:7442 
> > Change-Id: I27c43dd4293ce5de921be6c78571e712778d138a
> > Reviewed-on: https://chromium-review.googlesource.com/914610
> > Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
> > Commit-Queue: Adam Klein <adamk@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#51254}
>
> Bug:  v8:7442 ,  chromium:814182 
> Change-Id: I228c59dccef3844803f115749e72ae6c5f286eda
> Reviewed-on: https://chromium-review.googlesource.com/938241
> Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
> Commit-Queue: Adam Klein <adamk@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#51668}

Tbr: gsathya@chromium.org
Bug:  v8:7442 ,  v8:7524 ,  chromium:814182 ,  chromium:818627 ,  chromium:818672 ,  chromium:819994 ,  chromium:821788 
Change-Id: Ib760d63f879613f3b874889c5cb29ba2a77ba430
Reviewed-on: https://chromium-review.googlesource.com/980795
Commit-Queue: Adam Klein <adamk@chromium.org>
Reviewed-by: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52233}
[modify] https://crrev.com/5cf2ae5a4d7224198a0474eb27cc618b6843bc32/src/parsing/parser-base.h
[modify] https://crrev.com/5cf2ae5a4d7224198a0474eb27cc618b6843bc32/src/parsing/parser.h
[modify] https://crrev.com/5cf2ae5a4d7224198a0474eb27cc618b6843bc32/src/parsing/preparser.h

Comment 13 by adamk@chromium.org, Mar 27 2018

Status: Fixed (was: Assigned)

Sign in to add a comment