New issue
Advanced search Search tips

Issue 823473 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 10
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-10-31
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

1.7%-16% regression in blink_perf.svg at 543661:543812

Project Member Reported by m...@chromium.org, Mar 19 2018

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Mar 19 2018

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

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


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

android-nexus5
android-nexus5X
android-nexus6
android-nexus7v2
android-one
android-webview-nexus6
chromium-rel-mac11-air
chromium-rel-win7-dual
chromium-rel-win7-gpu-ati
chromium-rel-win7-gpu-nvidia
chromium-rel-win7-x64-dual
chromium-rel-win8-dual
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Mar 19 2018

Cc: pdr@chromium.org f...@opera.com
Owner: f...@opera.com
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/16fe3c59440000

<b>[CI] Stop tracking <*Gradient> 'href' references through SVGResources</b> by fs@opera.com
https://chromium.googlesource.com/chromium/src/+/ce851e47bd77773970a805acd62ec1627b0aaa76

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

Comment 4 by f...@opera.com, Mar 20 2018

Components: Blink>SVG
Project Member

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

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

commit 2df427946b090559b8a9a66c03a3b032e28f6511
Author: Fredrik Söderquist <fs@opera.com>
Date: Tue Mar 20 17:49:33 2018

[PE] Don't use callbacks for SVGElement::NotifyIncomingReferences

Dispatch overhead is high.

Bug:  661598 , 769774,  823473 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I4b200d0076c209584a033be810f95c504e81c102
Reviewed-on: https://chromium-review.googlesource.com/970422
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#544431}
[modify] https://crrev.com/2df427946b090559b8a9a66c03a3b032e28f6511/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.cpp
[modify] https://crrev.com/2df427946b090559b8a9a66c03a3b032e28f6511/third_party/WebKit/Source/core/svg/SVGElement.cpp
[modify] https://crrev.com/2df427946b090559b8a9a66c03a3b032e28f6511/third_party/WebKit/Source/core/svg/SVGElement.h
[modify] https://crrev.com/2df427946b090559b8a9a66c03a3b032e28f6511/third_party/WebKit/Source/core/svg/SVGGradientElement.cpp

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 23 2018

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

commit 2e4b433f9dac35b965e797247b8d15f287cad05c
Author: Fredrik Söderquist <fs@opera.com>
Date: Fri Mar 23 16:03:25 2018

[PE] Restore LayoutObject check in SVGElement::NotifyIncomingReferences

When an element has incoming references that are not in the layout tree
(like an element animated by [multiple] SMIL elements), the overhead of
the hash set insert/erase can be noticable (like for instance in
balls_svg_animation.html.)

Bug:  661598 , 769774,  823473 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I8a5879186dabd5e071f708ac953b8f1e91b62847
Reviewed-on: https://chromium-review.googlesource.com/978213
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#545467}
[modify] https://crrev.com/2e4b433f9dac35b965e797247b8d15f287cad05c/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceContainer.cpp
[modify] https://crrev.com/2e4b433f9dac35b965e797247b8d15f287cad05c/third_party/WebKit/Source/core/svg/SVGElement.h

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 24 2018

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

commit f2ead72d0feb6aec5347016ee852e84b7c72aee6
Author: Fredrik Söderquist <fs@opera.com>
Date: Sat Mar 24 10:22:03 2018

[PE] Don't reparse "restart" and "fill" repeatedly

Rather than doing an attribute lookup in GetRestart() and Fill(), just
map the attributes in ParseAttribute.
Also get rid of the static AtomicStrings.

Noticed while looking at the referenced bug.

Bug:  823473 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I19d1244c6034430c30e65c09e20c8f04552b1e5f
Reviewed-on: https://chromium-review.googlesource.com/978246
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#545686}
[modify] https://crrev.com/f2ead72d0feb6aec5347016ee852e84b7c72aee6/third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp
[modify] https://crrev.com/f2ead72d0feb6aec5347016ee852e84b7c72aee6/third_party/WebKit/Source/core/svg/animation/SVGSMILElement.h

Project Member

Comment 8 by bugdroid1@chromium.org, Mar 24 2018

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

commit 3e18a7bb42ff4d92525c60b74963f9554d956b5f
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Sat Mar 24 13:02:15 2018

Revert "[PE] Don't reparse "restart" and "fill" repeatedly"

This reverts commit f2ead72d0feb6aec5347016ee852e84b7c72aee6.

Reason for revert: Appears to be cause of compile failure on WIN MSVC64 (dbg):

https://ci.chromium.org/buildbot/chromium.win/WinMSVC64%20(dbg)/4171

../..\base/logging.h(778): error C2220: warning treated as error - no 'object' file generated
../../third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp(1095): note: see reference to function template instantiation 'std::string *logging::CheckEQImpl<unsigned int,blink::SVGSMILElement::ActiveState>(const t1 &,const t2 &,const char *)' being compiled
        with
        [
            t1=unsigned int,
            t2=blink::SVGSMILElement::ActiveState
        ]
../..\base/logging.h(778): warning C4389: '==': signed/unsigned mismatch
../..\base/logging.h(779): warning C4389: '!=': signed/unsigned mismatch
../../third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp(1101): note: see reference to function template instantiation 'std::string *logging::CheckNEImpl<unsigned int,blink::SVGSMILElement::ActiveState>(const t1 &,const t2 &,const char *)' being compiled
        with
        [
            t1=unsigned int,
            t2=blink::SVGSMILElement::ActiveState
        ]

Original change's description:
> [PE] Don't reparse "restart" and "fill" repeatedly
> 
> Rather than doing an attribute lookup in GetRestart() and Fill(), just
> map the attributes in ParseAttribute.
> Also get rid of the static AtomicStrings.
> 
> Noticed while looking at the referenced bug.
> 
> Bug:  823473 
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
> Change-Id: I19d1244c6034430c30e65c09e20c8f04552b1e5f
> Reviewed-on: https://chromium-review.googlesource.com/978246
> Reviewed-by: Philip Rogers <pdr@chromium.org>
> Commit-Queue: Fredrik Söderquist <fs@opera.com>
> Cr-Commit-Position: refs/heads/master@{#545686}

TBR=pdr@chromium.org,fs@opera.com,schenney@chromium.org

Change-Id: I78e7691c5753d6d5ef25249d0cb3a5fff0b517c6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  823473 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Reviewed-on: https://chromium-review.googlesource.com/979413
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545687}
[modify] https://crrev.com/3e18a7bb42ff4d92525c60b74963f9554d956b5f/third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp
[modify] https://crrev.com/3e18a7bb42ff4d92525c60b74963f9554d956b5f/third_party/WebKit/Source/core/svg/animation/SVGSMILElement.h

Project Member

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

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

commit 9c5a4f94f9c5920ed1e6ad0d33dd1b9d30991403
Author: Fredrik Söderquist <fs@opera.com>
Date: Mon Mar 26 14:47:11 2018

Reland "[PE] Don't reparse "restart" and "fill" repeatedly"

Rather than doing an attribute lookup in GetRestart() and Fill(), just
map the attributes in ParseAttribute. Pack with active_state_.
Also get rid of the static AtomicStrings.

Noticed while looking at the referenced bug.

Use new accessor GetActiveState() for active_state_ accesses to avoid
stumbling over signedness mismatches.

Bug:  823473 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I9adf72cdfcc794885b79be1bfc374554f9382d11
Reviewed-on: https://chromium-review.googlesource.com/979807
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#545786}
[modify] https://crrev.com/9c5a4f94f9c5920ed1e6ad0d33dd1b9d30991403/third_party/WebKit/Source/core/svg/animation/SVGSMILElement.cpp
[modify] https://crrev.com/9c5a4f94f9c5920ed1e6ad0d33dd1b9d30991403/third_party/WebKit/Source/core/svg/animation/SVGSMILElement.h

NextAction: 2018-10-31
Was this issue resolved? Can we mark it as Fixed?
Status: Fixed (was: Assigned)
I believe so.
The NextAction date has arrived: 2018-10-31

Sign in to add a comment