CSA fast path for Map.prototype.(Get|Has) for string keys.

CSA fast path for Map.prototype.(Get|Has) for string keys.

Review-Url: https://codereview.chromium.org/2964633002
Cr-Commit-Position: refs/heads/master@{#46394}
Committed: https://chromium.googlesource.com/v8/v8/+/3a243d3aef982126c36f00d9ea98b908e0490692

[Jarin, 2017-06-30 09:25:38]

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

[commit-bot: I haz the power, 2017-06-30 09:25:45]

Dry run: CQ is trying da patch.

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

[Jarin, 2017-06-30 09:36:36]

jarin@chromium.org changed reviewers:
+ cbruni@chromium.org

[Jarin, 2017-06-30 09:36:37]

Up for discussion.

Some observations:

- I am not sure it is worth to have fast path for strings. It is possible
(likely?) that general case in CSA would not be much slower.

- At the moment the hash-code computation calls runtime if the hash is not
there. Do you think it makes sense to replace with a C-call?

[commit-bot: I haz the power, 2017-06-30 09:59:40]

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

[commit-bot: I haz the power, 2017-06-30 09:59:41]

Dry run: Try jobs failed on following builders:
  v8_node_linux64_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_node_linux64_rel/builds/...)

[Jarin, 2017-06-30 11:09:01]

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

[commit-bot: I haz the power, 2017-06-30 11:09:10]

Dry run: 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, 2017-06-30 11:35:11]

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

[commit-bot: I haz the power, 2017-06-30 11:35:12]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2017-07-03 14:51:29]

Aside from possible code-bloat, I like the code this way...
C++ is still cumbersome with function pointers and and such ;)

LGTM

https://codereview.chromium.org/2964633002/diff/20001/src/builtins/builtins-c...
File src/builtins/builtins-collections-gen.cc (right):

https://codereview.chromium.org/2964633002/diff/20001/src/builtins/builtins-c...
src/builtins/builtins-collections-gen.cc:42: Label* entry_found, Variable*
entry_start_position, Label* not_found);
nit: I think it would be nice to have the entry_found and not_found next to each
other.

https://codereview.chromium.org/2964633002/diff/20001/src/builtins/builtins-c...
src/builtins/builtins-collections-gen.cc:465: Node* tagged_hash =
CallRuntime(Runtime::kGenericHash, context, string_key);
For me this RT-call is OK.

I remembered jkummerow complaining about that the hash-computation is rather
expensive. I don't remember whether he concluded that you can basically
internalize the key at slightly higher cost. (again that would mostly make sense
for Set)

https://codereview.chromium.org/2964633002/diff/20001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (left):

https://codereview.chromium.org/2964633002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:1223: GotoIf(IsClearWord32(hash_field,
Name::kHashNotComputedMask),
doh! that might have been me during a refactoring...

[Jarin, 2017-07-04 04:24:47]

https://codereview.chromium.org/2964633002/diff/20001/src/builtins/builtins-c...
File src/builtins/builtins-collections-gen.cc (right):

https://codereview.chromium.org/2964633002/diff/20001/src/builtins/builtins-c...
src/builtins/builtins-collections-gen.cc:42: Label* entry_found, Variable*
entry_start_position, Label* not_found);
On 2017/07/03 14:51:29, Camillo Bruni wrote:
> nit: I think it would be nice to have the entry_found and not_found next to
each
> other.

Well, the idea behind this order was that entry_start_position is only valid in
the entry_found, so those two go together. Since it was not clear to you, it
will likely not be clear to anyone; so I am moving it as you suggested.

https://codereview.chromium.org/2964633002/diff/20001/src/builtins/builtins-c...
src/builtins/builtins-collections-gen.cc:465: Node* tagged_hash =
CallRuntime(Runtime::kGenericHash, context, string_key);
On 2017/07/03 14:51:29, Camillo Bruni wrote:
> For me this RT-call is OK.
> 
> I remembered jkummerow complaining about that the hash-computation is rather
> expensive. I don't remember whether he concluded that you can basically
> internalize the key at slightly higher cost. (again that would mostly make
sense
> for Set)

Acknowledged.

https://codereview.chromium.org/2964633002/diff/20001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (left):

https://codereview.chromium.org/2964633002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:1223: GotoIf(IsClearWord32(hash_field,
Name::kHashNotComputedMask),
On 2017/07/03 14:51:29, Camillo Bruni wrote:
> doh! that might have been me during a refactoring...

Acknowledged.

[Jarin, 2017-07-04 04:24:54]

The CQ bit was checked by jarin@chromium.org

[Jarin, 2017-07-04 04:24:54]

The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org
Link to the patchset: https://codereview.chromium.org/2964633002/#ps40001
(title: "Address reviewer comments")

[commit-bot: I haz the power, 2017-07-04 04:25: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, 2017-07-04 04:51:55]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499142294287270,
"parent_rev": "93557496a2315111165b1b6f29ef5b5ba60b1837", "commit_rev":
"3a243d3aef982126c36f00d9ea98b908e0490692"}

[commit-bot: I haz the power, 2017-07-04 04:52:04]

Description was changed from

==========
CSA fast path for Map.prototype.(Get|Has) for string keys.
==========

to

==========
CSA fast path for Map.prototype.(Get|Has) for string keys.

Review-Url: https://codereview.chromium.org/2964633002
Cr-Commit-Position: refs/heads/master@{#46394}
Committed:
https://chromium.googlesource.com/v8/v8/+/3a243d3aef982126c36f00d9ea98b908e04...
==========

[commit-bot: I haz the power, 2017-07-04 04:52:06]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/3a243d3aef982126c36f00d9ea98b908e04...