[builtins] HasOwnProperty: handle non-internalized string keys

[builtins] HasOwnProperty: handle non-internalized string keys

Taking the slow runtime path for every non-internalized string key
can be avoided by doing optimistic string table lookups: if there
is a matching entry, use that; if there isn't, then no existing
object has a property with that name.
The hashing/internalizing logic is in C++ and called directly.

Review-Url: https://codereview.chromium.org/2811333002
Cr-Commit-Position: refs/heads/master@{#44650}
Committed: https://chromium.googlesource.com/v8/v8/+/204989a5aa3f6546aa01419cf8d24e7fca1b39f5

[Jakob Kummerow, 2017-04-12 16:56:18]

jkummerow@chromium.org changed reviewers:
+ bmeurer@chromium.org

[Jakob Kummerow, 2017-04-12 16:56:18]

PTAL. In a local microbenchmark around cons string creation and
.hasOwnProperty() calls, this is about 20-30% faster than ToT.

[Jakob Kummerow, 2017-04-12 16:56:58]

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

[commit-bot: I haz the power, 2017-04-12 16:57:07]

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-04-12 17:23:22]

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

[commit-bot: I haz the power, 2017-04-12 17:23:23]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2017-04-13 09:37:30]

LGTM on compiler, thanks!

[Igor Sheludko, 2017-04-13 11:38:53]

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

[Igor Sheludko, 2017-04-13 11:38:54]

lgtm with nits:

https://codereview.chromium.org/2811333002/diff/20001/src/ic/accessor-assembl...
File src/ic/accessor-assembler.cc (right):

https://codereview.chromium.org/2811333002/diff/20001/src/ic/accessor-assembl...
src/ic/accessor-assembler.cc:2118: /* {
I'd suggest to add a read-only bool option and do
  if (FLAG_internalize_on_the_fly) {

instead of commenting a block. This way we can at least be sure that the code
compiles.

Or even better, if you move this code to CSA::TryLookupInternalizedString(Node*
string, Label* if_index, Label* if_internalized, Label* if_never_internalized,
Label* if_bailout)

then you can reuse in "if_notunique_name" case of ObjectHasOwnProperty.

https://codereview.chromium.org/2811333002/diff/20001/src/ic/accessor-assembl...
src/ic/accessor-assembler.cc:2128: Node* int_result = SmiUntag(result);
s/int_result/word_result/ ?

https://codereview.chromium.org/2811333002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2811333002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:17600: string->set_hash_field(hash_field);
Maybe also hash_ = string->Hash(); instead of manual shifting above?

https://codereview.chromium.org/2811333002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:17711: return
Smi::FromInt(String::ArrayIndexValueBits::decode(hash));
It would be nice to add asserts somewhere that array index value can't be equal
to any of ResultSentinel values.
Maybe you can mark BitFieldBase::is_valid() as a constexpr method and add
something like

STATIC_ASSERT(!String::ArrayIndexValueBits::is_valid(ResultSentinel::kNotFound));
STATIC_ASSERT(!String::ArrayIndexValueBits::is_valid(ResultSentinel::kUnsupported));

[Jakob Kummerow, 2017-04-13 14:01:53]

Thanks for the reviews. Comments addressed, will rebase and land.

https://codereview.chromium.org/2811333002/diff/20001/src/ic/accessor-assembl...
File src/ic/accessor-assembler.cc (right):

https://codereview.chromium.org/2811333002/diff/20001/src/ic/accessor-assembl...
src/ic/accessor-assembler.cc:2118: /* {
On 2017/04/13 11:38:54, Igor Sheludko wrote:
> I'd suggest to add a read-only bool option and do
>   if (FLAG_internalize_on_the_fly) {
> 
> instead of commenting a block. This way we can at least be sure that the code
> compiles.
> 
> Or even better, if you move this code to
CSA::TryLookupInternalizedString(Node*
> string, Label* if_index, Label* if_internalized, Label* if_never_internalized,
> Label* if_bailout)
> 
> then you can reuse in "if_notunique_name" case of ObjectHasOwnProperty.

Done.

https://codereview.chromium.org/2811333002/diff/20001/src/ic/accessor-assembl...
src/ic/accessor-assembler.cc:2128: Node* int_result = SmiUntag(result);
On 2017/04/13 11:38:53, Igor Sheludko wrote:
> s/int_result/word_result/ ?

Done.

https://codereview.chromium.org/2811333002/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2811333002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:17600: string->set_hash_field(hash_field);
On 2017/04/13 11:38:54, Igor Sheludko wrote:
> Maybe also hash_ = string->Hash(); instead of manual shifting above?

Done.

https://codereview.chromium.org/2811333002/diff/20001/src/objects.cc#newcode1...
src/objects.cc:17711: return
Smi::FromInt(String::ArrayIndexValueBits::decode(hash));
On 2017/04/13 11:38:54, Igor Sheludko wrote:
> It would be nice to add asserts somewhere that array index value can't be
equal
> to any of ResultSentinel values.
> Maybe you can mark BitFieldBase::is_valid() as a constexpr method and add
> something like
> 
>
STATIC_ASSERT(!String::ArrayIndexValueBits::is_valid(ResultSentinel::kNotFound));
>
STATIC_ASSERT(!String::ArrayIndexValueBits::is_valid(ResultSentinel::kUnsupported));
> 

Done.

[Jakob Kummerow, 2017-04-13 14:03:49]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2017-04-13 14:03:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, ishell@chromium.org
Link to the patchset: https://codereview.chromium.org/2811333002/#ps60001
(title: "rebased (noop)")

[commit-bot: I haz the power, 2017-04-13 14:03:53]

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-04-13 14:41:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492092229644250,
"parent_rev": "fa0066d170f72a7cf5669213e6af5f6398e52bfb", "commit_rev":
"204989a5aa3f6546aa01419cf8d24e7fca1b39f5"}

[commit-bot: I haz the power, 2017-04-13 14:41:29]

Description was changed from

==========
[builtins] HasOwnProperty: handle non-internalized string keys

Taking the slow runtime path for every non-internalized string key
can be avoided by doing optimistic string table lookups: if there
is a matching entry, use that; if there isn't, then no existing
object has a property with that name.
The hashing/internalizing logic is in C++ and called directly.
==========

to

==========
[builtins] HasOwnProperty: handle non-internalized string keys

Taking the slow runtime path for every non-internalized string key
can be avoided by doing optimistic string table lookups: if there
is a matching entry, use that; if there isn't, then no existing
object has a property with that name.
The hashing/internalizing logic is in C++ and called directly.

Review-Url: https://codereview.chromium.org/2811333002
Cr-Commit-Position: refs/heads/master@{#44650}
Committed:
https://chromium.googlesource.com/v8/v8/+/204989a5aa3f6546aa01419cf8d24e7fca1...
==========

[commit-bot: I haz the power, 2017-04-13 14:41:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/204989a5aa3f6546aa01419cf8d24e7fca1...