New issue
Advanced search Search tips

Issue 606211 link

Starred by 3 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Feature

Blocked on:
issue 605792
issue 605701
issue 606210



Sign in to add a comment

[meta] Animometer - Leaves test tracker

Project Member Reported by vmi...@chromium.org, Apr 24 2016

Issue description

.
 

Comment 1 by vmi...@chromium.org, Apr 25 2016

Labels: Hotlist-Animometer

Comment 2 by jianli@chromium.org, Apr 25 2016

Owner: vmi...@chromium.org
vmiura, please provide more details on this new bug. Please also label this bug with right Blink area.

Comment 3 by vmi...@chromium.org, Apr 25 2016

This issue is for tracking improvements to the Animometer Leaves benchmark.  Please see the blocking issues for each item.

There are multiple issues in Blink.  What would be a good Blink label?

To run this benchmark:

1) Visit https://pr.gg/animometer/developer.html
2) Select Animometer > Leaves.
3) Run Benchmark


Summary: [meta] Animometer - Leaves test tracker (was: Animometer - Leaves test tracker)

Comment 5 by yutak@chromium.org, Apr 28 2016

You can set multiple Blink sub-components in that case.
Labels: -Restrict-View-Google
Project Member

Comment 7 by bugdroid1@chromium.org, May 24 2016

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

commit 5f88403bf3e53a955cee48e172269d0ed94f2595
Author: esprehn <esprehn@chromium.org>
Date: Tue May 24 16:10:00 2016

Fix whitespace handling in parseSimpleTransformList

The loop would break if there was any trailing whitespace after the
args of the transform operation function, but this meant that if you
put a space between the functions like 'translateX(1px) translateY(1px)'
you'd fall out of the fast path when hitting the space between the
two functions. Any trailing space would similarly fall out of the fast
path and result in wasting all of the work building the transform list
only to discard it.

This patch instead skips whitespace between the functions and also adds
a bunch of tests for the fast paths.

BUG=606211

Review-Url: https://codereview.chromium.org/2007823002
Cr-Commit-Position: refs/heads/master@{#395610}

[modify] https://crrev.com/5f88403bf3e53a955cee48e172269d0ed94f2595/third_party/WebKit/Source/core/core.gypi
[modify] https://crrev.com/5f88403bf3e53a955cee48e172269d0ed94f2595/third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp
[modify] https://crrev.com/5f88403bf3e53a955cee48e172269d0ed94f2595/third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.h
[add] https://crrev.com/5f88403bf3e53a955cee48e172269d0ed94f2595/third_party/WebKit/Source/core/css/parser/CSSParserFastPathsTest.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, May 25 2016

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

commit ad21c7423bc912e4c5287204d7c1eb57b994099a
Author: esprehn <esprehn@chromium.org>
Date: Wed May 25 04:13:54 2016

Add an early abort for the CSSParserFastPaths transform path.

For transforms that miss the fast path like
"transform(1px, 1px) rotate(1deg)" we end up spending a lot of time
building the transform list and doing string to double conversions for
the function calls we do understand that were listed before the ones we
didn't understand. This work ends up wasted and we have to redo it all
inside the real CSSParser later.

This patch introduces a scan over the string that tries to reject all
transform things we don't understand in advance.

This is not the best, since ideally we'd just remove the fast path and
make the real parser just as fast, but in the meantime this at least
avoids doing lots of wasted work.

This patch makes the script in Animometer > Leaves 8% faster.

The change is covered by the tests introduced in:
https://codereview.chromium.org/2007823002

BUG=606211

Review-Url: https://codereview.chromium.org/1996343003
Cr-Commit-Position: refs/heads/master@{#395797}

[modify] https://crrev.com/ad21c7423bc912e4c5287204d7c1eb57b994099a/third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, May 25 2016

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

commit d56e7dd93c7a25a390ebda860dcd17a17533d5f8
Author: esprehn <esprehn@chromium.org>
Date: Wed May 25 05:44:39 2016

Avoid calling lower() on inline styles in parseKeywordValue.

We were doing string.lower() on every inline style that was assigned
to a non-keyword property to compare the value to "inherit" and
"initial". This is wasteful and shows up in profiles, instead lets use
equalIgnoringASCIICase.

To do this we need to expand the overload set for
equalIgnoringASCIICase to include one that takes a const char* so that
we don't allocate a temporary String instance when passing a literal.
We also inline the call to strlen so that the compiler can figure out
the length for us and avoid calling strlen at runtime.

This reduces the script execution time of Animometer > Leaves by 1% on
my Macbook Pro.

BUG=606211

Review-Url: https://codereview.chromium.org/2006413002
Cr-Commit-Position: refs/heads/master@{#395809}

[modify] https://crrev.com/d56e7dd93c7a25a390ebda860dcd17a17533d5f8/third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp
[modify] https://crrev.com/d56e7dd93c7a25a390ebda860dcd17a17533d5f8/third_party/WebKit/Source/core/css/parser/CSSParserFastPathsTest.cpp
[modify] https://crrev.com/d56e7dd93c7a25a390ebda860dcd17a17533d5f8/third_party/WebKit/Source/wtf/text/StringImpl.cpp
[modify] https://crrev.com/d56e7dd93c7a25a390ebda860dcd17a17533d5f8/third_party/WebKit/Source/wtf/text/StringImpl.h
[modify] https://crrev.com/d56e7dd93c7a25a390ebda860dcd17a17533d5f8/third_party/WebKit/Source/wtf/text/WTFString.h

Project Member

Comment 10 by bugdroid1@chromium.org, May 25 2016

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

commit 1252d54ec614bb49f87a25c60fd5ce74eaa87be4
Author: robertocn <robertocn@chromium.org>
Date: Wed May 25 19:19:25 2016

Revert of Add an early abort for the CSSParserFastPaths transform path. (patchset #5 id:80001 of https://codereview.chromium.org/1996343003/ )

Reason for revert:
This might be causing a timeout in several perf tests.

Original issue's description:
> Add an early abort for the CSSParserFastPaths transform path.
>
> For transforms that miss the fast path like
> "transform(1px, 1px) rotate(1deg)" we end up spending a lot of time
> building the transform list and doing string to double conversions for
> the function calls we do understand that were listed before the ones we
> didn't understand. This work ends up wasted and we have to redo it all
> inside the real CSSParser later.
>
> This patch introduces a scan over the string that tries to reject all
> transform things we don't understand in advance.
>
> This is not the best, since ideally we'd just remove the fast path and
> make the real parser just as fast, but in the meantime this at least
> avoids doing lots of wasted work.
>
> This patch makes the script in Animometer > Leaves 8% faster.
>
> The change is covered by the tests introduced in:
> https://codereview.chromium.org/2007823002
>
> BUG=606211
>
> Committed: https://crrev.com/ad21c7423bc912e4c5287204d7c1eb57b994099a
> Cr-Commit-Position: refs/heads/master@{#395797}

TBR=meade@chromium.org,timloh@chromium.org,dstockwell@chromium.org,esprehn@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=606211

Review-Url: https://codereview.chromium.org/2009723003
Cr-Commit-Position: refs/heads/master@{#395957}

[modify] https://crrev.com/1252d54ec614bb49f87a25c60fd5ce74eaa87be4/third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp

Project Member

Comment 11 by bugdroid1@chromium.org, May 27 2016

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

commit 1e57a010caf64fcc35b942644e466220176a0d05
Author: esprehn <esprehn@chromium.org>
Date: Fri May 27 01:07:00 2016

Add an early abort for the CSSParserFastPaths transform path.

For transforms that miss the fast path like
"transform(1px, 1px) rotate(1deg)" we end up spending a lot of time
building the transform list and doing string to double conversions for
the function calls we do understand that were listed before the ones we
didn't understand. This work ends up wasted and we have to redo it all
inside the real CSSParser later.

This patch introduces a scan over the string that tries to reject all
transform things we don't understand in advance.

This is not the best, since ideally we'd just remove the fast path and
make the real parser just as fast, but in the meantime this at least
avoids doing lots of wasted work.

This patch makes the script in Animometer > Leaves 8% faster.

The change is covered by the tests introduced in:
https://codereview.chromium.org/2007823002

This was originally committed as:
https://crrev.com/ad21c7423bc912e4c5287204d7c1eb57b994099a
but contained a bug where we were doing WTF::find() + 1 which meant
that when the string was not found we'd end up doing unsigned(-1) + 1
which wraps around to 0 causing an infinite loop. I fixed it by
checking for kNotFound explicitly.

BUG=606211

Review-Url: https://codereview.chromium.org/1996343003
Cr-Commit-Position: refs/heads/master@{#396338}

[modify] https://crrev.com/1e57a010caf64fcc35b942644e466220176a0d05/third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp
[modify] https://crrev.com/1e57a010caf64fcc35b942644e466220176a0d05/third_party/WebKit/Source/core/css/parser/CSSParserFastPathsTest.cpp

Components: -Blink
Project Member

Comment 13 by sheriffbot@chromium.org, Jul 12 2017

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 14 by enne@chromium.org, Jul 21 2017

Status: Assigned (was: Untriaged)
vmiura, is this fixed?

Comment 15 by ojan@chromium.org, May 8 2018

Cc: -ojan@chromium.org

Sign in to add a comment