NOT FOR LANDING: Hack up CSSParser for speed.

NOT FOR LANDING: Hack up CSSParser for speed.

This saves 18-35% depending on what you're parsing. It totally disables the fast
path though which exposes that parsing some things (ex. colors) don't improve with
this set of changes, but parsing things like transform when you miss the fast path
are quite a lot better. If we left in the fast path that would be bad since it 
penalizes all callers to .transform = "" that miss the fast path.

We should continue to explore more ideas to speed up the parser, if the fast
path can parse a color 3x faster today, we totally want that when parsing sheets
too and not just when parsing .style.foo assignments.

BUG=605792

[esprehn, 2016-04-22 23:27:44]

Description was changed from

==========
Hack up CSSParser for speed.
==========

to

==========
NOT FOR LANDING: Hack up CSSParser for speed.

This saves 18-35% depending on what you're parsing. It totally disables the fast
path though which exposes that parsing some things (ex. colors) are much worse
still, but parsing things like transform when you miss the fast path are quite
a lot better. If we left in the fast path that would be bad since it penalizes
all callers to .transform = "" that miss the fast path.

BUG=605792
==========

[esprehn, 2016-04-22 23:30:49]

Description was changed from

==========
NOT FOR LANDING: Hack up CSSParser for speed.

This saves 18-35% depending on what you're parsing. It totally disables the fast
path though which exposes that parsing some things (ex. colors) are much worse
still, but parsing things like transform when you miss the fast path are quite
a lot better. If we left in the fast path that would be bad since it penalizes
all callers to .transform = "" that miss the fast path.

BUG=605792
==========

to

==========
NOT FOR LANDING: Hack up CSSParser for speed.

This saves 18-35% depending on what you're parsing. It totally disables the fast
path though which exposes that parsing some things (ex. colors) don't improve
with
this set of changes, but parsing things like transform when you miss the fast
path
are quite a lot better. If we left in the fast path that would be bad since it 
penalizes all callers to .transform = "" that miss the fast path.

We should continue to explore more ideas to speed up the parser, if the fast
path can parse a color 3x faster today, we totally want that when parsing sheets
too and not just when parsing .style.foo assignments.

BUG=605792
==========

[esprehn, 2016-04-22 23:30:58]

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp (right):

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp:90:
CSSPrimitiveValue::UnitType toUnitType(CharacterType* chars, unsigned length)
This was a very big improvement, it skips malloc of the String and the hash
table lookup.

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSParserImpl.h (right):

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserImpl.h:115: const
CSSParserContext& m_context;
This only saved a tiny bit, but every copy counts.

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSParserTokenRange.h (left):

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserTokenRange.h:23:
CSSParserTokenRange(const Vector<CSSParserToken>& vector)
We could also use a template I suppose to allow inline capacity here. I think
this should probably be explicit though.

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:141: // if
(hasInvalidNumericValues(range))
I think Tab is going to remove this. Doing a scan over the whole token range
looking for numbers is bad. I think this code actually scans it twice too, once
in parseValue and again in parseSingleValue called later. We should probably
clamp or abort inside the tokenizer if this is something we actually want to
enforce.

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp (right):

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp:373: return
CSSParserToken(DelimiterToken, cc);
Skip the virtual call overhead of the lookup table. This also means we don't
need to pass arguments into the functions like comma() which don't need it.

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSTokenizer.h (right):

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSTokenizer.h:38:
Vector<CSSParserToken, 32> m_tokens;
Lots of malloc in the profiles. By using inline capacity here I saved a bunch of
time.

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSTokenizer.h:83:
Vector<CSSParserTokenType, 8> m_blockStack;
ditto

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSTokenizerInputStream.h (left):

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSTokenizerInputStream.h:21: inline
UChar nextInputChar()
missing const on both, also don't need inline in the class body

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSTokenizerInputStream.h:38: double
getDouble(unsigned start, unsigned end);
missing const

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSTokenizerInputStream.h:53: const
size_t m_stringLength;
There's no reason to store the length, we have it in m_string.

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSTokenizerInputStream.h
(right):

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSTokenizerInputStream.h:17:
DISALLOW_NEW();
always on the stack or a member :)

[Timothy Loh, 2016-04-27 06:52:05]

I know this particular CL isn't for landing, but I had comments anyway...

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp (right):

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp:96: if
(toASCIILower(chars[0]) == 's')
toASCIILowerUnchecked or isASCIIAlphaCaselessEqual (I'm not sure why this
function has a LIKELY)

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp:200: // "__qem"
this isn't exposed, if it helps we can rename it to be shorter ;)

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSParserTokenRange.h (left):

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSParserTokenRange.h:23:
CSSParserTokenRange(const Vector<CSSParserToken>& vector)
On 2016/04/22 23:30:57, esprehn wrote:
> We could also use a template I suppose to allow inline capacity here. I think
> this should probably be explicit though.

IIRC I set it up so that once you have a CSSParserTokenRange you can only create
additional ranges by using makeSubRange. I think this was so that if you
consume() past the end and get the staticEOF token you can still make a range
and it gets the correct last value.

A template is probably fine because you can (I think?) write template<size_t
inlineBuffer> CSSParserTokenRange(const Vector<CSSParserToken, inlineBuffer>&
vector). Maybe it'd be easier to just using CSSParserTokenVector =
Vector<CSSParserToken, 32> and use that everywhere though.

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp (right):

https://codereview.chromium.org/1920583002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/parser/CSSTokenizer.cpp:373: return
CSSParserToken(DelimiterToken, cc);
On 2016/04/22 23:30:58, esprehn wrote:
> Skip the virtual call overhead of the lookup table. This also means we don't
> need to pass arguments into the functions like comma() which don't need it.

This is definitely an improvement (I think using code generation to make the
lookup table is just unneeded complexity), but I'm not convinced this will
actually make any performance difference.

[dglazkov, 2016-04-27 15:51:28]

On 2016/04/27 at 06:52:05, timloh wrote:
> This is definitely an improvement (I think using code generation to make the
lookup table is just unneeded complexity), but I'm not convinced this will
actually make any performance difference.

Didn't Elliott state the factual performance difference in the CL description?
Confused about the "actually make any performance difference" statement.

[Timothy Loh, 2016-04-28 01:39:57]

On 2016/04/27 15:51:28, dglazkov wrote:
> On 2016/04/27 at 06:52:05, timloh wrote:
> > This is definitely an improvement (I think using code generation to make the
> lookup table is just unneeded complexity), but I'm not convinced this will
> actually make any performance difference.
> 
> Didn't Elliott state the factual performance difference in the CL description?
> Confused about the "actually make any performance difference" statement.

Just the particular bit of code the comment was attached to, not the change as a
whole.

[esprehn, 2016-04-28 01:52:00]

On 2016/04/28 at 01:39:57, timloh wrote:
> On 2016/04/27 15:51:28, dglazkov wrote:
> > On 2016/04/27 at 06:52:05, timloh wrote:
> > > This is definitely an improvement (I think using code generation to make
the
> > lookup table is just unneeded complexity), but I'm not convinced this will
> > actually make any performance difference.
> > 
> > Didn't Elliott state the factual performance difference in the CL
description?
> > Confused about the "actually make any performance difference" statement.
> 
> Just the particular bit of code the comment was attached to, not the change as
a whole.

It's true that that part of the change was a small improvement, but it was an
improvement, and it might be more noticeable on ARM. :)

Certainly we should start with the unit parsing and inline capacity, those were
the big wins.

[dglazkov, 2016-04-29 01:28:54]

On 2016/04/28 at 01:39:57, timloh wrote:
> On 2016/04/27 15:51:28, dglazkov wrote:
> > On 2016/04/27 at 06:52:05, timloh wrote:
> > > This is definitely an improvement (I think using code generation to make
the
> > lookup table is just unneeded complexity), but I'm not convinced this will
> > actually make any performance difference.
> > 
> > Didn't Elliott state the factual performance difference in the CL
description?
> > Confused about the "actually make any performance difference" statement.
> 
> Just the particular bit of code the comment was attached to, not the change as
a whole.

D'oh.