New issue
Advanced search Search tips

Issue 819994 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

1%-1.7% regression in system_health.memory_desktop at 540547:540745

Project Member Reported by petermarshall@chromium.org, Mar 8 2018

Issue description

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

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


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

android-nexus5
android-webview-nexus6
chromium-rel-mac11-air
chromium-rel-mac11-pro
chromium-rel-mac12
chromium-rel-win7-gpu-ati
chromium-rel-win7-gpu-nvidia
chromium-rel-win7-x64-dual
chromium-rel-win8-dual
linux-release
Cc: ishell@chromium.org cbruni@chromium.org adamk@chromium.org gsat...@chromium.org
Owner: adamk@chromium.org
Status: Assigned (was: Untriaged)
📍 Found significant differences after each of 2 commits.
https://pinpoint-dot-chromeperf.appspot.com/job/14901d1a440000

[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

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions

Comment 4 by adamk@chromium.org, Mar 8 2018

I'm rather confused by this bug. The title says "system_health.memory_mobile", but both the chromeperf link and the pinpoint job show "system_health.memory_desktop". petermarshall, any idea why such a discrepancy?
Summary: 1%-1.7% regression in system_health.memory_desktop at 540547:540745 (was: 1%-1.7% regression in system_health.memory_mobile at 540547:540745)
So it looks like there are 2 regressions in this group for system_health.memory_mobile, and one of them was at the top of the list when I filed the bug so that pre-filled into the name. I'm not sure why the automatic bisect doesn't pick the same regression from the group as the automatic bug title...

In any case I'll rename it to memory_desktop given the majority of the regressions in the group are related to that. I also kicked off a bisect for one of the mobile ones specifically in case it is different, although the range given in chromeperf is the same.
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, Mar 22 2018

😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/14d49a9a440000
Project Member

Comment 8 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 9 by adamk@chromium.org, Mar 27 2018

Status: Fixed (was: Assigned)

Sign in to add a comment