New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 905745 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

2.3% regression in media.desktop at 607313:607361

Project Member Reported by hubbe@chromium.org, Nov 15

Issue description

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

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


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

linux-perf

media.desktop - Benchmark documentation link:
  None
Cc: pbos@chromium.org
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/10e1517be40000

Interpolate loading progress in tab animation by pbos@chromium.org
https://chromium.googlesource.com/chromium/src/+/98d67224c1af355a7578341060a591d2b6505578
memory:chrome:all_processes:reported_by_chrome:cc:effective_size: 1.227e+07 → 1.199e+07 (-2.897e+05)

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

Benchmark documentation link:
  None
Cc: -pbos@chromium.org hubbe@chromium.org
Owner: pbos@chromium.org
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 6

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

commit 355b8185716292a8f145c77168ee89148ecabf31
Author: Peter Boström <pbos@chromium.org>
Date: Thu Dec 06 00:44:43 2018

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}
[modify] https://crrev.com/355b8185716292a8f145c77168ee89148ecabf31/chrome/browser/ui/views/tabs/tab_icon.cc
[modify] https://crrev.com/355b8185716292a8f145c77168ee89148ecabf31/chrome/browser/ui/views/tabs/tab_unittest.cc
[modify] https://crrev.com/355b8185716292a8f145c77168ee89148ecabf31/chrome/common/chrome_features.cc

Status: Fixed (was: Assigned)
Graphs back down after r614199.
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 6

Labels: merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1cbde78f21bba5d565b93ac1236e9ff00ed7ff24

commit 1cbde78f21bba5d565b93ac1236e9ff00ed7ff24
Author: Peter Boström <pbos@chromium.org>
Date: Thu Dec 06 18:09:10 2018

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-Original-Commit-Position: refs/heads/master@{#614199}(cherry picked from commit 355b8185716292a8f145c77168ee89148ecabf31)
Reviewed-on: https://chromium-review.googlesource.com/c/1365983
Reviewed-by: Peter Boström <pbos@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#118}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/1cbde78f21bba5d565b93ac1236e9ff00ed7ff24/chrome/browser/ui/views/tabs/tab_icon.cc
[modify] https://crrev.com/1cbde78f21bba5d565b93ac1236e9ff00ed7ff24/chrome/browser/ui/views/tabs/tab_unittest.cc
[modify] https://crrev.com/1cbde78f21bba5d565b93ac1236e9ff00ed7ff24/chrome/common/chrome_features.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Dec 6

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

commit 742b8857c4dd0ab4550f7b2c1f200ce5be406b9c
Author: Jeffrey Yasskin <jyasskin@chromium.org>
Date: Thu Dec 06 19:13:17 2018

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}
[modify] https://crrev.com/742b8857c4dd0ab4550f7b2c1f200ce5be406b9c/chrome/browser/ui/views/tabs/tab_icon.cc
[modify] https://crrev.com/742b8857c4dd0ab4550f7b2c1f200ce5be406b9c/chrome/browser/ui/views/tabs/tab_unittest.cc
[modify] https://crrev.com/742b8857c4dd0ab4550f7b2c1f200ce5be406b9c/chrome/common/chrome_features.cc

Project Member

Comment 9 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

Project Member

Comment 10 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 11 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