AstValueFactory: add a cache for one-character strings.

AstValueFactory: add a cache for one-character strings.

Lowercase 1 character strings occur frequently in minified code. Add a
cache for them, so that we don't need to compute the hash + do the hash
table lookup for each occurrence.

BUG=

Committed: https://crrev.com/f36497d2c901e548685c40962b0cabcfddae4cda
Cr-Commit-Position: refs/heads/master@{#41597}

[marja, 2016-12-01 12:43:34]

marja@chromium.org changed reviewers:
+ verwaest@chromium.org

[marja, 2016-12-01 12:43:35]

wdyt?

With this approach, we only save computing the hash + the hash table lookup (for
cases when the string is already in the cache).

We still need the hash for internalizing. Also, we could avoid copying the
string into the Zone, but we'd then need some trickery for cases which want to
get the string's content; I don't think it's worth it.

The try jobs don't show any clear improvement with this CL.

[marja, 2016-12-01 12:45:06]

The CQ bit was checked by marja@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 12:45:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marja, 2016-12-01 12:47:47]

(Forgot to say; I check that in minified code (example pulled from optimize.js)
lower case 1-char identifiers occur like 10 x more frequently than upper case
1-char identifiers, so I don't think it's worth it to extend the cache
(diminishing returns).)

[commit-bot: I haz the power, 2016-12-01 13:08:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 13:08:56]

Dry run: This issue passed the CQ dry run.

[Toon Verwaest, 2016-12-05 15:30:41]

lgtm

https://codereview.chromium.org/2541353002/diff/1/src/ast/ast-value-factory.cc
File src/ast/ast-value-factory.cc (right):

https://codereview.chromium.org/2541353002/diff/1/src/ast/ast-value-factory.c...
src/ast/ast-value-factory.cc:225: if (literal.length() == 1 && literal[0] >= 'a'
&& literal[0] <= 'z') {
IsInRange(literal[0], 'a', 'z') is faster ;)

https://codereview.chromium.org/2541353002/diff/1/src/ast/ast-value-factory.c...
src/ast/ast-value-factory.cc:228: uint32_t hash =
StringHasher::HashSequentialString<uint8_t>(
We could just have a-z heapstrings preallocated in a similar table and just
manually fill in without internalization lookup in the symbol table. Or in a
follow-up CL :)

https://codereview.chromium.org/2541353002/diff/1/src/ast/ast-value-factory.h
File src/ast/ast-value-factory.h (right):

https://codereview.chromium.org/2541353002/diff/1/src/ast/ast-value-factory.h...
src/ast/ast-value-factory.h:436: AstRawString* one_character_strings_[26];
constants ... :) I guess the alphabet won't change any time soon...

[marja, 2016-12-08 15:09:59]

thanks for review!

https://codereview.chromium.org/2541353002/diff/1/src/ast/ast-value-factory.cc
File src/ast/ast-value-factory.cc (right):

https://codereview.chromium.org/2541353002/diff/1/src/ast/ast-value-factory.c...
src/ast/ast-value-factory.cc:225: if (literal.length() == 1 && literal[0] >= 'a'
&& literal[0] <= 'z') {
On 2016/12/05 15:30:40, Toon Verwaest wrote:
> IsInRange(literal[0], 'a', 'z') is faster ;)

Done.

https://codereview.chromium.org/2541353002/diff/1/src/ast/ast-value-factory.c...
src/ast/ast-value-factory.cc:228: uint32_t hash =
StringHasher::HashSequentialString<uint8_t>(
On 2016/12/05 15:30:40, Toon Verwaest wrote:
> We could just have a-z heapstrings preallocated in a similar table and just
> manually fill in without internalization lookup in the symbol table. Or in a
> follow-up CL :)

Hmm yeah, sounds like a good idea to keep the CLs separate. (I'd also like to
see the potential perf gain from this CL alone.)

[marja, 2016-12-08 15:13:00]

The CQ bit was checked by marja@chromium.org

[marja, 2016-12-08 15:13:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2541353002/#ps40001
(title: "code review (verwaest@)")

[commit-bot: I haz the power, 2016-12-08 15:13:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 15:39:35]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481209980295330,
"parent_rev": "2291197e40aa0f8b38b419a7d8f3d7a7f2f9bbdf", "commit_rev":
"b9ea9e46a58c5513c186d21fb7e068403f3f8ac3"}

[commit-bot: I haz the power, 2016-12-08 15:39:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-08 15:40:09]

Description was changed from

==========
AstValueFactory: add a cache for one-character strings.

Lowercase 1 character strings occur frequently in minified code. Add a
cache for them, so that we don't need to compute the hash + do the hash
table lookup for each occurrence.

BUG=
==========

to

==========
AstValueFactory: add a cache for one-character strings.

Lowercase 1 character strings occur frequently in minified code. Add a
cache for them, so that we don't need to compute the hash + do the hash
table lookup for each occurrence.

BUG=

Committed: https://crrev.com/f36497d2c901e548685c40962b0cabcfddae4cda
Cr-Commit-Position: refs/heads/master@{#41597}
==========

[commit-bot: I haz the power, 2016-12-08 15:40:09]

Patchset 3 (id:??) landed as
https://crrev.com/f36497d2c901e548685c40962b0cabcfddae4cda
Cr-Commit-Position: refs/heads/master@{#41597}