Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1996343003:
Add an early abort for the CSSParserFastPaths transform path. (Closed)
|
Created:
4 years, 7 months ago by esprehn Modified:
4 years, 7 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, chromium-reviews, dglazkov+blink, rwlbuis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd 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
Committed: https://crrev.com/1e57a010caf64fcc35b942644e466220176a0d05
Cr-Commit-Position: refs/heads/master@{#396338}
Patch Set 1 #Patch Set 2 : do it right. #Patch Set 3 : simpler. #Patch Set 4 : Add tests, also remove parseKeywordValue from the profile. #Patch Set 5 : Break out other opts. #Patch Set 6 : Handle missing parens correctly. #
Messages
Total messages: 37 (20 generated)
Description was changed from ========== 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. BUG=605792,606211 ========== to ========== 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. BUG=605792,606211 ==========
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996343003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996343003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== 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. BUG=605792,606211 ========== to ========== 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. Also included fixes: - Stop lower casing the string in parseKeywordValue. - Add tests!!!! This patch makes the script in Animometer > Leaves 8% faster. BUG=605792,606211 ==========
Description was changed from ========== 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. Also included fixes: - Stop lower casing the string in parseKeywordValue. - Add tests!!!! This patch makes the script in Animometer > Leaves 8% faster. BUG=605792,606211 ========== to ========== NOT FOR COMMIT: Add an early abort for the CSSParserFastPaths transform path. THIS WILL BE LANDED IN PARTS. 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. Also included fixes: - Stop lower casing the string in parseKeywordValue. - Add tests!!!! This patch makes the script in Animometer > Leaves 8% faster. BUG=605792,606211 ==========
Description was changed from ========== NOT FOR COMMIT: Add an early abort for the CSSParserFastPaths transform path. THIS WILL BE LANDED IN PARTS. 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. Also included fixes: - Stop lower casing the string in parseKeywordValue. - Add tests!!!! This patch makes the script in Animometer > Leaves 8% faster. BUG=605792,606211 ========== to ========== 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. BUG=606211 ==========
Description was changed from ========== 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. BUG=606211 ========== to ========== 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 ==========
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996343003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996343003/80001
esprehn@chromium.org changed reviewers: + meade@chromium.org, timloh@chromium.org
dstockwell@chromium.org changed reviewers: + dstockwell@chromium.org
lgtm
The CQ bit was unchecked by esprehn@chromium.org
The CQ bit was checked by esprehn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996343003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996343003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ad21c7423bc912e4c5287204d7c1eb57b994099a Cr-Commit-Position: refs/heads/master@{#395797}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2009723003/ by robertocn@chromium.org. The reason for reverting is: This might be causing a timeout in several perf tests..
timloh@ Want to look at this again? I fixed my bug and added a test. :)
Description was changed from ========== 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} ========== to ========== 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 ==========
Description was changed from ========== 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 ========== to ========== 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 ==========
On 2016/05/26 20:52:48, esprehn wrote: > timloh@ Want to look at this again? I fixed my bug and added a test. :) lgtm
The CQ bit was checked by esprehn@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996343003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996343003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
The CQ bit was checked by esprehn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dstockwell@chromium.org Link to the patchset: https://codereview.chromium.org/1996343003/#ps100001 (title: "Handle missing parens correctly.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1996343003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1996343003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/1e57a010caf64fcc35b942644e466220176a0d05 Cr-Commit-Position: refs/heads/master@{#396338} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/1e57a010caf64fcc35b942644e466220176a0d05 Cr-Commit-Position: refs/heads/master@{#396338} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
