More efficient code for mapping event names and attribute names.

More efficient code for mapping event names and attribute names.

This changes repeated code to a loop and cuts
HTMLElement::eventNameForAttributeName from 9 KB to 3 KB.

clang, x64:

Total change: -5952 bytes
=========================
  1 shrunk, for a net change of -5952 bytes (9338 bytes before, 3386 bytes after) across 1 sources

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182820

[Daniel Bratell, 2014-09-25 17:20:18]

bratell@opera.com changed reviewers:
+ pdr@chromium.org

[Daniel Bratell, 2014-09-25 17:20:19]

pdr, could you please take a look at this or suggest some reviewer if you think
someone else should take a look. It just reorders the code to be less "sparse",
making the function much smaller.

[pdr., 2014-09-26 03:43:29]

pdr@chromium.org changed reviewers:
+ abarth@chromium.org

[pdr., 2014-09-26 04:07:16]

I think we can do even better by not creating this map dynamically. I poked
around the code and didn't find a precedent for something better though.

@abarth, can you recommend an approach here?

https://codereview.chromium.org/594413003/diff/1/Source/core/html/HTMLElement...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/594413003/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:224: { onabortAttr, EventTypeNames::abort},
Nit: add a space before the curly braces.

[Daniel Bratell, 2014-09-26 08:10:51]

Since the tables (this and other similar ones) contain references to objects
(AtomicStrings, QualifiedNames) they can't be generated fully compile time. For
that we would need to store something without a constructor, that the compiler
could create in space.

Simplest (since that is what many other systems do) idea would be to switch from
AtomicString objects to AtomicString "numbers", most likely offsets into some
kind of global table. Unless I've missed something that would be a rather large
redesign though. The code likes its string-AtomicStrings.

Some version which stores constant 8-bit strings could also work, but that would
either rely on the compiler/linker merging all occurrences of a constant string,
or have to use string comparisons when mixed with real string objects, or
possibly both.

But please take a new look, I've added the spaces.

https://codereview.chromium.org/594413003/diff/1/Source/core/html/HTMLElement...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/594413003/diff/1/Source/core/html/HTMLElement...
Source/core/html/HTMLElement.cpp:224: { onabortAttr, EventTypeNames::abort},
On 2014/09/26 04:07:15, pdr wrote:
> Nit: add a space before the curly braces.

Done.

[pdr., 2014-09-26 23:21:11]

On 2014/09/26 08:10:51, Daniel Bratell wrote:
> Since the tables (this and other similar ones) contain references to objects
> (AtomicStrings, QualifiedNames) they can't be generated fully compile time.
For
> that we would need to store something without a constructor, that the compiler
> could create in space.
> 
> Simplest (since that is what many other systems do) idea would be to switch
from
> AtomicString objects to AtomicString "numbers", most likely offsets into some
> kind of global table.

Isn't that what atomic strings are? The trick would be to pre-populate the
atomic string table at compile time with these constants and use their hash in a
switch statement.

I think this is a fine change (can't argue with smaller binary!). I saw you have
a few more patches with this pattern. If you don't mind, can we give abarth
another day or two to respond before landing them? He solved similar problems
(e.g., make_token_matcher.py) in writing the css parser and we may know of a
better pattern here.

[abarth-chromium, 2014-09-28 00:55:11]

On 2014/09/26 at 23:21:11, pdr wrote:
> I think this is a fine change (can't argue with smaller binary!). I saw you
have a few more patches with this pattern. If you don't mind, can we give abarth
another day or two to respond before landing them? He solved similar problems
(e.g., make_token_matcher.py) in writing the css parser and we may know of a
better pattern here.

I don't know how to do better with maps.  One thing we've used in other places
is to use binary search over a static table rather than a map.  However, I think
this CL is fine as an incremental step.

[pdr., 2014-09-28 01:32:04]

LGTM

[pdr., 2014-09-28 01:32:10]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-09-28 01:34:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/594413003/20001

[esprehn, 2014-09-29 00:22:05]

On 2014/09/28 at 01:34:08, commit-bot wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/594413003/20001

The primary savings here is that the hash function and all the HashMap setting
logic is inlined into every callsite. We might want to reconsider that.

[esprehn, 2014-09-29 00:23:03]

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

[esprehn, 2014-09-29 00:23:04]

https://codereview.chromium.org/594413003/diff/20001/Source/core/html/HTMLEle...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/594413003/diff/20001/Source/core/html/HTMLEle...
Source/core/html/HTMLElement.cpp:312:
attributeNameToEventNameMap.set(attrToEventNames[i].attr.localName(),
attrToEventNames[i].event);
Why not ditch the hash table entirely and use std::lower_bound like RenderQuote
does?

[Daniel Bratell, 2014-09-29 07:50:33]

About HashTables, I did some experiments a few months ago and it was about 300
KB of inlined hash code (see https://codereview.chromium.org/373423003/ ). I've
been planning to revisit it with performance measurements and similar.

About using binary search over a static table instead of a hashtable, I think
that might be a better solution, but it takes me a lot of time to produce the
evidence that changes like that don't cause a performance regression. Too much
time for a few KB as long as there are bigger fish.

[commit-bot: I haz the power, 2014-09-29 09:36:18]

Committed patchset #2 (id:20001) as 182820