New issue
Advanced search Search tips

Issue 913135 link

Starred by 1 user

Issue metadata

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


Show other hotlists

Hotlists containing this issue:
Media


Sign in to add a comment

5.9%-14.7% regression in media.desktop at 614430:614493

Project Member Reported by chcunningham@chromium.org, Dec 7

Issue description

Another case where we're returning to a previous value from a couple hundred commits back. Expecting noise or possible something got reverted.
 
All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=913135

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


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

linux-perf
mac-10_13_laptop_high_end-perf

media.desktop - Benchmark documentation link:
  None
Cc: jyasskin@chromium.org
Owner: jyasskin@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/1419b0c4140000

Revert "Disable the new tab-loading animation" by jyasskin@chromium.org
https://chromium.googlesource.com/chromium/src/+/742b8857c4dd0ab4550f7b2c1f200ce5be406b9c
cpu_time_percentage: 0.3346 → 0.3936 (+0.05899)

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

Benchmark documentation link:
  None
Cc: pbos@chromium.org
Owner: pbos@chromium.org
I expect pbos@ will be rolling that change forward again, and ought to know that the new tab-loading animation may cause performance regressions.
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ad12d44124812f22484eb4d123e4a3b4aaac93b9

commit ad12d44124812f22484eb4d123e4a3b4aaac93b9
Author: Peter Boström <pbos@chromium.org>
Date: Tue Dec 11 18:02:22 2018

Reland "Disable the new tab-loading animation"

This reverts commit 742b8857c4dd0ab4550f7b2c1f200ce5be406b9c.

Reason for revert: Flaky tests should be fixed in r615470.

Bug:  chromium:912543 ,  chromium:913135 ,  chromium:913784 

Original change's description:
> Revert "Disable the new tab-loading animation"
> 
> This reverts commit 355b8185716292a8f145c77168ee89148ecabf31.
> 
> Reason for revert: Made several tests flaky:  https://crbug.com/912543 
> 
> Original change's description:
> > Disable the new tab-loading animation
> > 
> > Makes sure that a lot of animation-related code is bypassed when the
> > new-tab-animation flag is off. This should hopefully fix a couple of
> > performance regressions that have not yet been root caused so that they
> > don't go out with M72.
> > 
> > Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> > Change-Id: Id3f131db427eb3ee1618d6c9683fd5e47dc134e8
> > Reviewed-on: https://chromium-review.googlesource.com/c/1364212
> > Reviewed-by: Sidney San MartĂ­n <sdy@chromium.org>
> > Commit-Queue: Peter Boström <pbos@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#614199}
> 
> TBR=pbos@chromium.org,sdy@chromium.org
> 
> Change-Id: Ib4c022a255ad085c1716d3559a7f84dcb61c2785
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> Reviewed-on: https://chromium-review.googlesource.com/c/1366359
> Reviewed-by: Jeffrey Yasskin <jyasskin@chromium.org>
> Commit-Queue: Jeffrey Yasskin <jyasskin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#614440}

TBR=jyasskin@chromium.org,pbos@chromium.org,sdy@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
Change-Id: I3981455a00f743fae535de4626179dcceab9b2c4
Reviewed-on: https://chromium-review.googlesource.com/c/1372236
Reviewed-by: Peter Boström <pbos@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615581}
[modify] https://crrev.com/ad12d44124812f22484eb4d123e4a3b4aaac93b9/chrome/browser/ui/views/tabs/tab_icon.cc
[modify] https://crrev.com/ad12d44124812f22484eb4d123e4a3b4aaac93b9/chrome/browser/ui/views/tabs/tab_unittest.cc
[modify] https://crrev.com/ad12d44124812f22484eb4d123e4a3b4aaac93b9/chrome/common/chrome_features.cc

Status: Fixed (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6921bd825e416c7b412b454f60df8b183e906943

commit 6921bd825e416c7b412b454f60df8b183e906943
Author: Friedrich Horschig <fhorschig@chromium.org>
Date: Wed Dec 12 14:10:33 2018

Revert "Reland "Disable the new tab-loading animation""

This reverts commit ad12d44124812f22484eb4d123e4a3b4aaac93b9.

Reason for revert:
Findit found this to be the most likely culprit for the single_process_mash_browser_tests failures on various Linux bots:
https://findit-for-me.appspot.com/waterfall/failure?url=https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/30518


Original change's description:
> Reland "Disable the new tab-loading animation"
>
> This reverts commit 742b8857c4dd0ab4550f7b2c1f200ce5be406b9c.
>
> Reason for revert: Flaky tests should be fixed in r615470.
>
> Bug:  chromium:912543 ,  chromium:913135 ,  chromium:913784 
>
> Original change's description:
> > Revert "Disable the new tab-loading animation"
> >
> > This reverts commit 355b8185716292a8f145c77168ee89148ecabf31.
> >
> > Reason for revert: Made several tests flaky:  https://crbug.com/912543 
> >
> > Original change's description:
> > > Disable the new tab-loading animation
> > >
> > > Makes sure that a lot of animation-related code is bypassed when the
> > > new-tab-animation flag is off. This should hopefully fix a couple of
> > > performance regressions that have not yet been root caused so that they
> > > don't go out with M72.
> > >
> > > Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> > > Change-Id: Id3f131db427eb3ee1618d6c9683fd5e47dc134e8
> > > Reviewed-on: https://chromium-review.googlesource.com/c/1364212
> > > Reviewed-by: Sidney San MartĂ­n <sdy@chromium.org>
> > > Commit-Queue: Peter Boström <pbos@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#614199}
> >
> > TBR=pbos@chromium.org,sdy@chromium.org
> >
> > Change-Id: Ib4c022a255ad085c1716d3559a7f84dcb61c2785
> > No-Presubmit: true
> > No-Tree-Checks: true
> > No-Try: true
> > Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> > Reviewed-on: https://chromium-review.googlesource.com/c/1366359
> > Reviewed-by: Jeffrey Yasskin <jyasskin@chromium.org>
> > Commit-Queue: Jeffrey Yasskin <jyasskin@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#614440}
>
> TBR=jyasskin@chromium.org,pbos@chromium.org,sdy@chromium.org
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> Change-Id: I3981455a00f743fae535de4626179dcceab9b2c4
> Reviewed-on: https://chromium-review.googlesource.com/c/1372236
> Reviewed-by: Peter Boström <pbos@chromium.org>
> Commit-Queue: Peter Boström <pbos@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#615581}

TBR=jyasskin@chromium.org,pbos@chromium.org,sdy@chromium.org

Change-Id: Ib9599bb8fd44327ae756d3c9523049367409612c
Bug:  chromium:912543 ,  chromium:913135 ,  chromium:913784 ,  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
Reviewed-on: https://chromium-review.googlesource.com/c/1373832
Commit-Queue: Friedrich Horschig [CET] <fhorschig@chromium.org>
Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615878}
[modify] https://crrev.com/6921bd825e416c7b412b454f60df8b183e906943/chrome/browser/ui/views/tabs/tab_icon.cc
[modify] https://crrev.com/6921bd825e416c7b412b454f60df8b183e906943/chrome/browser/ui/views/tabs/tab_unittest.cc
[modify] https://crrev.com/6921bd825e416c7b412b454f60df8b183e906943/chrome/common/chrome_features.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 13

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/831dad6e5036b31b683587078db3df8e32ce4f92

commit 831dad6e5036b31b683587078db3df8e32ce4f92
Author: Peter Boström <pbos@chromium.org>
Date: Thu Dec 13 16:50:04 2018

Reland "Reland "Disable the new tab-loading animation""

This reverts commit 6921bd825e416c7b412b454f60df8b183e906943.

Reason for revert: NOTE TO SHERIFFS: Both the revert and reland of this
CL is the culprit for FindIt flakes. Context:

This CL changes the duration of the tab-icon animation ("spinner"). This
likely has some effects on test loops that RunUntilIdle or similar, so a
pending request might either have happened or not before that loop gets
idle.

For instance:  crbug.com/912543  had to be changed to ignore a favicon.ico
response. This flakiness could be reproduced by just changing
TabIcon::ShowingLoadingAnimation to "return false;" even though the test
does not have anything to do with Chrome's UI directly.

This reland will likely trigger crbug.com/914232 again, but the revert
triggered  crbug.com/914644 . It's not unlikely that they are both flaky
as they implicitly rely on tab-icon load time.

Bug: chromium:914232,  chromium:914644 

Original change's description:
> Revert "Reland "Disable the new tab-loading animation""
> 
> This reverts commit ad12d44124812f22484eb4d123e4a3b4aaac93b9.
> 
> Reason for revert:
> Findit found this to be the most likely culprit for the single_process_mash_browser_tests failures on various Linux bots:
> https://findit-for-me.appspot.com/waterfall/failure?url=https://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/builds/30518
> 
> 
> Original change's description:
> > Reland "Disable the new tab-loading animation"
> >
> > This reverts commit 742b8857c4dd0ab4550f7b2c1f200ce5be406b9c.
> >
> > Reason for revert: Flaky tests should be fixed in r615470.
> >
> > Bug:  chromium:912543 ,  chromium:913135 ,  chromium:913784 
> >
> > Original change's description:
> > > Revert "Disable the new tab-loading animation"
> > >
> > > This reverts commit 355b8185716292a8f145c77168ee89148ecabf31.
> > >
> > > Reason for revert: Made several tests flaky:  https://crbug.com/912543 
> > >
> > > Original change's description:
> > > > Disable the new tab-loading animation
> > > >
> > > > Makes sure that a lot of animation-related code is bypassed when the
> > > > new-tab-animation flag is off. This should hopefully fix a couple of
> > > > performance regressions that have not yet been root caused so that they
> > > > don't go out with M72.
> > > >
> > > > Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> > > > Change-Id: Id3f131db427eb3ee1618d6c9683fd5e47dc134e8
> > > > Reviewed-on: https://chromium-review.googlesource.com/c/1364212
> > > > Reviewed-by: Sidney San MartĂ­n <sdy@chromium.org>
> > > > Commit-Queue: Peter Boström <pbos@chromium.org>
> > > > Cr-Commit-Position: refs/heads/master@{#614199}
> > >
> > > TBR=pbos@chromium.org,sdy@chromium.org
> > >
> > > Change-Id: Ib4c022a255ad085c1716d3559a7f84dcb61c2785
> > > No-Presubmit: true
> > > No-Tree-Checks: true
> > > No-Try: true
> > > Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> > > Reviewed-on: https://chromium-review.googlesource.com/c/1366359
> > > Reviewed-by: Jeffrey Yasskin <jyasskin@chromium.org>
> > > Commit-Queue: Jeffrey Yasskin <jyasskin@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#614440}
> >
> > TBR=jyasskin@chromium.org,pbos@chromium.org,sdy@chromium.org
> >
> > # Not skipping CQ checks because original CL landed > 1 day ago.
> >
> > Bug:  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> > Change-Id: I3981455a00f743fae535de4626179dcceab9b2c4
> > Reviewed-on: https://chromium-review.googlesource.com/c/1372236
> > Reviewed-by: Peter Boström <pbos@chromium.org>
> > Commit-Queue: Peter Boström <pbos@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#615581}
> 
> TBR=jyasskin@chromium.org,pbos@chromium.org,sdy@chromium.org
> 
> Change-Id: Ib9599bb8fd44327ae756d3c9523049367409612c
> Bug:  chromium:912543 ,  chromium:913135 ,  chromium:913784 ,  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
> Reviewed-on: https://chromium-review.googlesource.com/c/1373832
> Commit-Queue: Friedrich Horschig [CET] <fhorschig@chromium.org>
> Reviewed-by: Friedrich Horschig [CET] <fhorschig@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#615878}

TBR=jyasskin@chromium.org,pbos@chromium.org,sdy@chromium.org,fhorschig@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  chromium:912543 ,  chromium:913135 ,  chromium:913784 ,  chromium:912328 ,  chromium:905745 ,  chromium:905918 , chromium:910265
Change-Id: Ib8083b1320f3e1cd5c878bd55d2a007af7bf719a
Reviewed-on: https://chromium-review.googlesource.com/c/1376030
Reviewed-by: Peter Boström <pbos@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616334}
[modify] https://crrev.com/831dad6e5036b31b683587078db3df8e32ce4f92/chrome/browser/ui/views/tabs/tab_icon.cc
[modify] https://crrev.com/831dad6e5036b31b683587078db3df8e32ce4f92/chrome/browser/ui/views/tabs/tab_unittest.cc
[modify] https://crrev.com/831dad6e5036b31b683587078db3df8e32ce4f92/chrome/common/chrome_features.cc

Sign in to add a comment