Generate a series of nested switch statements to parse CSSPrimitiveValue units.

Generate a series of nested switch statements to parse CSSPrimitiveValue units.

First, read in a text file of strings to types, generate a Trie, then
use the Trie to generate the nested switch statements.

Generated CSSPrimitiveValueUnitTrie.cpp:
http://pastebin.com/EMJU9eNp

Using the instructions here:
https://chromium.googlesource.com/chromium/src/+/master/docs/linux_profiling.md
when running the "slow" part of the inline-transform
benchmark linked in http://crbug.com/605792,
it looks like this change halves the time spent in
blink::CSSTokenizer::consumeNumericToken, taking 10% off
CSSParserImpl::parseValue.

BUG=606695

[Timothy Loh, 2016-05-03 07:40:24]

timloh@chromium.org changed reviewers:
+ timloh@chromium.org

[Timothy Loh, 2016-05-03 07:40:26]

comments as per discussion (maybe better to put WIP in the patch title to make
it clear to others)

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/build/scripts/make_css_primitive_value_unit_trie.py
(right):

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_primitive_value_unit_trie.py:40:
def generate_header(self):
unused

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_element_lookup_trie.py
(right):

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_element_lookup_trie.py:48:
dict(char, subtrie, string, value, conditions):
Can we return something structured like (e.g.)

{
 'a' : {
  'b' : ("cd", "abcd.impl()"..), 
  'd' : {
   'e' : {
    'f' : ("", "adef.impl()"),
    'g' : ("", "adeg.impl()"),
   },
  },
 },
}

Should be a fair amount simpler. I think you can differentiate between internal
and leaf nodes in jinja using mapping()

http://jinja.pocoo.org/docs/dev/templates/#mapping

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_element_lookup_trie.py:83: for k, g
in groupby(str_to_return_value_dict, char_at_index):
same comment as other place about groupby/OrderedDict

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_element_lookup_trie.py:91: def
_conditions(tag, index):
as discussed, let's move all of the logic for this into the template

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_element_lookup_trie.py:99:
ordered_dict = OrderedDict(
OrderedDict+groupby seems weird here, probably better to do

result = defaultdict(list)
for key,value in str_.._dict.items():
    result[len(key)].append((key, value))

which makes a dict structured like

{2: [("em",value1), ("px",value2)]}

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/macros.tmpl (right):

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/macros.tmpl:10: {% macro
trie_leaf(index, entry, return_macro, default_return_value) %}
We'll need a flag should_lower since unlike the tag names the dimension units
are not already lower case. Also it's potentially simpler to get rid of
return_macro and make the values stored in the tried the actual return values.

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/macros.tmpl:18: return
{{default_return_value}};
It might be possible to make this a "break" so we don't have to pass around
default_return_value but can just type it at the bottom of the functions in the
calling code.

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

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPrimitiveValueUnitTrie.h:12:
CSSPrimitiveValue::UnitType lookupCSSPrimitiveValueUnit(const String& data,
unsigned length);
reminder note that this still creates stings and we'll need to change this to
not do so

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPrimitiveValueUnits.in (right):

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPrimitiveValueUnits.in:11: ""
unit_type=UserUnits
let's just hard-code this in CSSPrimitiveValueUnitTrie.cpp.tmpl instead of
putting it in this file

[meade_UTC10, 2016-05-03 07:50:54]

Description was changed from

==========
Generate a series of nested switch statements to parse CSSPrimitiveValue units.

First, read in a text file of strings to types, generate a Trie, then
use the Trie to generate the nested switch statements.

BUG=606695
==========

to

==========
Generate a series of nested switch statements to parse CSSPrimitiveValue units.

First, read in a text file of strings to types, generate a Trie, then
use the Trie to generate the nested switch statements.

BUG=606695
==========

[meade_UTC10, 2016-05-04 07:24:54]

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/build/scripts/make_css_primitive_value_unit_trie.py
(right):

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_css_primitive_value_unit_trie.py:40:
def generate_header(self):
On 2016/05/03 07:40:25, Timothy Loh wrote:
> unused

Done.

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/make_element_lookup_trie.py
(right):

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_element_lookup_trie.py:48:
dict(char, subtrie, string, value, conditions):
On 2016/05/03 07:40:25, Timothy Loh wrote:
> Can we return something structured like (e.g.)
> 
> {
>  'a' : {
>   'b' : ("cd", "abcd.impl()"..), 
>   'd' : {
>    'e' : {
>     'f' : ("", "adef.impl()"),
>     'g' : ("", "adeg.impl()"),
>    },
>   },
>  },
> }
> 
> Should be a fair amount simpler. I think you can differentiate between
internal
> and leaf nodes in jinja using mapping()
> 
> http://jinja.pocoo.org/docs/dev/templates/#mapping

Done.

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_element_lookup_trie.py:83: for k, g
in groupby(str_to_return_value_dict, char_at_index):
On 2016/05/03 07:40:25, Timothy Loh wrote:
> same comment as other place about groupby/OrderedDict

Done.

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_element_lookup_trie.py:91: def
_conditions(tag, index):
On 2016/05/03 07:40:25, Timothy Loh wrote:
> as discussed, let's move all of the logic for this into the template

Done.

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/make_element_lookup_trie.py:99:
ordered_dict = OrderedDict(
On 2016/05/03 07:40:25, Timothy Loh wrote:
> OrderedDict+groupby seems weird here, probably better to do
> 
> result = defaultdict(list)
> for key,value in str_.._dict.items():
>     result[len(key)].append((key, value))
> 
> which makes a dict structured like
> 
> {2: [("em",value1), ("px",value2)]}

Done.

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/macros.tmpl (right):

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/macros.tmpl:18: return
{{default_return_value}};
On 2016/05/03 07:40:25, Timothy Loh wrote:
> It might be possible to make this a "break" so we don't have to pass around
> default_return_value but can just type it at the bottom of the functions in
the
> calling code.

Done.

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

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPrimitiveValueUnitTrie.h:12:
CSSPrimitiveValue::UnitType lookupCSSPrimitiveValueUnit(const String& data,
unsigned length);
On 2016/05/03 07:40:26, Timothy Loh wrote:
> reminder note that this still creates stings and we'll need to change this to
> not do so

Done.

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSPrimitiveValueUnits.in (right):

https://codereview.chromium.org/1938343002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSPrimitiveValueUnits.in:11: ""
unit_type=UserUnits
On 2016/05/03 07:40:26, Timothy Loh wrote:
> let's just hard-code this in CSSPrimitiveValueUnitTrie.cpp.tmpl instead of
> putting it in this file

Done.

[meade_UTC10, 2016-05-09 03:59:25]

Description was changed from

==========
Generate a series of nested switch statements to parse CSSPrimitiveValue units.

First, read in a text file of strings to types, generate a Trie, then
use the Trie to generate the nested switch statements.

BUG=606695
==========

to

==========
Generate a series of nested switch statements to parse CSSPrimitiveValue units.

First, read in a text file of strings to types, generate a Trie, then
use the Trie to generate the nested switch statements.

Generated CSSPrimitiveValueUnitTrie.cpp:
http://pastebin.com/U1x3HQYF

Diff between HTMLElementLookupTrie.cpp before and after:
http://www.diff-online.com/view/57300b111abe1
BUG=606695
==========

[meade_UTC10, 2016-05-09 04:13:57]

Description was changed from

==========
Generate a series of nested switch statements to parse CSSPrimitiveValue units.

First, read in a text file of strings to types, generate a Trie, then
use the Trie to generate the nested switch statements.

Generated CSSPrimitiveValueUnitTrie.cpp:
http://pastebin.com/U1x3HQYF

Diff between HTMLElementLookupTrie.cpp before and after:
http://www.diff-online.com/view/57300b111abe1
BUG=606695
==========

to

==========
Generate a series of nested switch statements to parse CSSPrimitiveValue units.

First, read in a text file of strings to types, generate a Trie, then
use the Trie to generate the nested switch statements.

Generated CSSPrimitiveValueUnitTrie.cpp:
http://pastebin.com/EMJU9eNp

Diff between HTMLElementLookupTrie.cpp before and after:
http://www.diff-online.com/view/57300e3ee815f

BUG=606695
==========

[meade_UTC10, 2016-05-11 05:59:41]

Description was changed from

==========
Generate a series of nested switch statements to parse CSSPrimitiveValue units.

First, read in a text file of strings to types, generate a Trie, then
use the Trie to generate the nested switch statements.

Generated CSSPrimitiveValueUnitTrie.cpp:
http://pastebin.com/EMJU9eNp

Diff between HTMLElementLookupTrie.cpp before and after:
http://www.diff-online.com/view/57300e3ee815f

BUG=606695
==========

to

==========
Generate a series of nested switch statements to parse CSSPrimitiveValue units.

First, read in a text file of strings to types, generate a Trie, then
use the Trie to generate the nested switch statements.

Generated CSSPrimitiveValueUnitTrie.cpp:
http://pastebin.com/EMJU9eNp

Diff between HTMLElementLookupTrie.cpp before and after:
http://www.diff-online.com/view/57300e3ee815f

Using the instructions here:
https://chromium.googlesource.com/chromium/src/+/master/docs/linux_profiling.md
when running the "slow" part of the inline-transform
benchmark linked in http://crbug.com/605792,
it looks like this change halves the time spent in
blink::CSSTokenizer::consumeNumericToken, taking 10% off
CSSParserImpl::parseValue.

BUG=606695
==========

[Timothy Loh, 2016-05-12 07:03:15]

As discussed, let's split this up into a patch for generalizing the switch logic
and one that actually changes the primitive unit types

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/make_css_primitive_value_unit_trie.py
(right):

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_css_primitive_value_unit_trie.py:29:
self._dict = {}
I'd just use a dict comprehension

self._units = {entry['name'] : entry['unit_type'] for entry in
self.in_file.name_dictionaries}

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/templates/CSSPrimitiveValueUnitTrie.cpp.tmpl
(right):

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/CSSPrimitiveValueUnitTrie.cpp.tmpl:2:
{% macro trie_return_statement(unit_name)
%}CSSPrimitiveValue::UnitType::{{unit_name}}{% endmacro %}
can this line be right before calling trie_length_switch?

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/templates/ElementLookupTrie.cpp.tmpl
(right):

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/ElementLookupTrie.cpp.tmpl:3:
{% macro trie_return_statement(tag) %}{{tag}}Tag.localName().impl(){% endmacro
%}
can this line go immediately before the trie_length_switch call?

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/ElementLookupTrie.cpp.tmpl:18:
{{ trie_length_switch(none, length_tries, trie_return_statement, '0') |
indent(4) }}
0 -> nullptr

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/templates/macros.tmpl (right):

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/macros.tmpl:53: {% if
empty_case_return_value %}
This shouldn't be needed any more due to
https://codereview.chromium.org/1952973002

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/macros.tmpl:62: return {{
default_return_value }};
I think it'd be nicer if the users of the macro just write the return
themselves.

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSPrimitiveValueUnits.in (right):

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPrimitiveValueUnits.in:32: 
unneeded blank line

[meade_UTC10, 2016-05-13 03:48:19]

These comments are addressed in https://codereview.chromium.org/1978703002

I will update the rest of the comments in this CL momentarily.

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/templates/ElementLookupTrie.cpp.tmpl
(right):

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/ElementLookupTrie.cpp.tmpl:3:
{% macro trie_return_statement(tag) %}{{tag}}Tag.localName().impl(){% endmacro
%}
On 2016/05/12 07:03:14, Timothy Loh wrote:
> can this line go immediately before the trie_length_switch call?

Done.

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/ElementLookupTrie.cpp.tmpl:18:
{{ trie_length_switch(none, length_tries, trie_return_statement, '0') |
indent(4) }}
On 2016/05/12 07:03:14, Timothy Loh wrote:
> 0 -> nullptr

Done.

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/build/scripts/templates/macros.tmpl (right):

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/macros.tmpl:53: {% if
empty_case_return_value %}
On 2016/05/12 07:03:14, Timothy Loh wrote:
> This shouldn't be needed any more due to
> https://codereview.chromium.org/1952973002

Done.

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/macros.tmpl:62: return {{
default_return_value }};
On 2016/05/12 07:03:14, Timothy Loh wrote:
> I think it'd be nicer if the users of the macro just write the return
> themselves.

Done.

[meade_UTC10, 2016-05-13 04:32:05]

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/make_css_primitive_value_unit_trie.py
(right):

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/make_css_primitive_value_unit_trie.py:29:
self._dict = {}
On 2016/05/12 07:03:14, Timothy Loh wrote:
> I'd just use a dict comprehension
> 
> self._units = {entry['name'] : entry['unit_type'] for entry in
> self.in_file.name_dictionaries}

Done.

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/templates/CSSPrimitiveValueUnitTrie.cpp.tmpl
(right):

https://codereview.chromium.org/1938343002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/CSSPrimitiveValueUnitTrie.cpp.tmpl:2:
{% macro trie_return_statement(unit_name)
%}CSSPrimitiveValue::UnitType::{{unit_name}}{% endmacro %}
On 2016/05/12 07:03:14, Timothy Loh wrote:
> can this line be right before calling trie_length_switch?

Done.

[meade_UTC10, 2016-05-13 04:33:11]

Description was changed from

==========
Generate a series of nested switch statements to parse CSSPrimitiveValue units.

First, read in a text file of strings to types, generate a Trie, then
use the Trie to generate the nested switch statements.

Generated CSSPrimitiveValueUnitTrie.cpp:
http://pastebin.com/EMJU9eNp

Diff between HTMLElementLookupTrie.cpp before and after:
http://www.diff-online.com/view/57300e3ee815f

Using the instructions here:
https://chromium.googlesource.com/chromium/src/+/master/docs/linux_profiling.md
when running the "slow" part of the inline-transform
benchmark linked in http://crbug.com/605792,
it looks like this change halves the time spent in
blink::CSSTokenizer::consumeNumericToken, taking 10% off
CSSParserImpl::parseValue.

BUG=606695
==========

to

==========
Generate a series of nested switch statements to parse CSSPrimitiveValue units.

First, read in a text file of strings to types, generate a Trie, then
use the Trie to generate the nested switch statements.

Generated CSSPrimitiveValueUnitTrie.cpp:
http://pastebin.com/EMJU9eNp

Using the instructions here:
https://chromium.googlesource.com/chromium/src/+/master/docs/linux_profiling.md
when running the "slow" part of the inline-transform
benchmark linked in http://crbug.com/605792,
it looks like this change halves the time spent in
blink::CSSTokenizer::consumeNumericToken, taking 10% off
CSSParserImpl::parseValue.

BUG=606695
==========

[Timothy Loh, 2016-05-13 06:54:48]

lgtm with all the default_value stuff removed, the patch rebased, and an updated
generated file linked to in the description

https://codereview.chromium.org/1938343002/diff/260001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/build/scripts/templates/CSSPrimitiveValueUnitTrie.cpp.tmpl
(right):

https://codereview.chromium.org/1938343002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/build/scripts/templates/CSSPrimitiveValueUnitTrie.cpp.tmpl:20:
return {{ trie_return_statement(default_value) }};
I think it's clearer to just return CSSPrimitiveValue::UnitType::Unknown, and
remove the default_value everywhere