[builtins] Properly optimize Object.prototype.isPrototypeOf.

[builtins] Properly optimize Object.prototype.isPrototypeOf.

Port the baseline implementation of Object.prototype.isPrototypeOf to
the CodeStubAssembler, sharing the existing prototype chain lookup logic
with the instanceof / OrdinaryHasInstance implementation. Based on that,
do the same in TurboFan, introducing a new JSHasInPrototypeChain
operator, which encapsulates the central prototype chain walk logic.

This speeds up Object.prototype.isPrototypeOf by more than a factor of
four, so that the code

  A.prototype.isPrototypeOf(a)

is now performance-wise on par with

  a instanceof A

for the case where A is a regular constructor function and a is an
instance of A.

Since instanceof does more than just the fundamental prototype chain
lookup, it was discovered in Node core that O.p.isPrototypeOf would
be a more appropriate alternative for certain sanity checks, since
it's less vulnerable to monkey-patching. In addition, the Object
builtin would also avoid the performance-cliff associated with
instanceof (due to the Symbol.hasInstance hook), as for example hit
by https://github.com/nodejs/node/pull/13403#issuecomment-305915874.
The main blocker was the missing performance of isPrototypeOf, since
it was still a JS builtin backed by a runtime call.

This CL also adds more test coverage for the
Object.prototype.isPrototypeOf builtin, especially when called from
optimized code.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng
BUG=v8:5269, v8:5989, v8:6483
R=jgruber@chromium.org

Review-Url: https://codereview.chromium.org/2934893002
Cr-Commit-Position: refs/heads/master@{#45925}
Committed: https://chromium.googlesource.com/v8/v8/+/b11c557d32846fde2615621ac7528e28dd992c86

[Benedikt Meurer, 2017-06-12 19:49:30]

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

[commit-bot: I haz the power, 2017-06-12 19:49:38]

Dry run: CQ is trying da patch.

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

[Benedikt Meurer, 2017-06-12 19:49:52]

Description was changed from

==========
[turbofan] Properly optimize Object.prototype.isPrototypeOf.

Port the baseline implementation of Object.prototype.isPrototypeOf to
the CodeStubAssembler, sharing the existing prototype chain lookup logic
with the instanceof / OrdinaryHasInstance implementation. Based on that,
do the same in TurboFan, introducing a new JSHasInPrototypeChain
operator, which encapsulates the central prototype chain walk logic.

This speeds up Object.prototype.isPrototypeOf by more than a factor of
four, so that the code

  A.prototype.isPrototypeOf(a)

is now performance-wise on par with

  a instanceof A

for the case where A is a regular constructor function and a is an
instance of A.

Since instanceof does more than just the fundamental prototype chain
lookup, it was discovered in Node core that O.p.isPrototypeOf would
be a more appropriate alternative for certain sanity checks, since
it's less vulnerable to monkey-patching. In addition, the Object
builtin would also avoid the performance-cliff associated with
instanceof (due to the Symbol.hasInstance hook), as for example hit
by https://github.com/nodejs/node/pull/13403#issuecomment-305915874.
The main blocker was the missing performance of isPrototypeOf, since
it was still a JS builtin backed by a runtime call.

This CL also adds more test coverage for the
Object.prototype.isPrototypeOf builtin, especially when called from
optimized code.

BUG=v8:5269,v8:5989,v8:6483
R=jgruber@chromium.org
==========

to

==========
[builtins] Properly optimize Object.prototype.isPrototypeOf.

Port the baseline implementation of Object.prototype.isPrototypeOf to
the CodeStubAssembler, sharing the existing prototype chain lookup logic
with the instanceof / OrdinaryHasInstance implementation. Based on that,
do the same in TurboFan, introducing a new JSHasInPrototypeChain
operator, which encapsulates the central prototype chain walk logic.

This speeds up Object.prototype.isPrototypeOf by more than a factor of
four, so that the code

  A.prototype.isPrototypeOf(a)

is now performance-wise on par with

  a instanceof A

for the case where A is a regular constructor function and a is an
instance of A.

Since instanceof does more than just the fundamental prototype chain
lookup, it was discovered in Node core that O.p.isPrototypeOf would
be a more appropriate alternative for certain sanity checks, since
it's less vulnerable to monkey-patching. In addition, the Object
builtin would also avoid the performance-cliff associated with
instanceof (due to the Symbol.hasInstance hook), as for example hit
by https://github.com/nodejs/node/pull/13403#issuecomment-305915874.
The main blocker was the missing performance of isPrototypeOf, since
it was still a JS builtin backed by a runtime call.

This CL also adds more test coverage for the
Object.prototype.isPrototypeOf builtin, especially when called from
optimized code.

BUG=v8:5269,v8:5989,v8:6483
R=jgruber@chromium.org
==========

[Benedikt Meurer, 2017-06-12 19:52:26]

Hey Jakob,

Here's follow-up work for the instanceof core functionality shared with
Object.prototype.isPrototypeOf. The motivation came from Node core, where checks
for prototype are often required, but instanceof comes with additional features
that you don't want/need.
A lot of this CL is basically refactoring what we already had (because of
instanceof). The new bits for Object.prototype.isPrototypeOf are mostly trivial
in the end.
Please take a look.

Thanks!
Benedikt

https://codereview.chromium.org/2934893002/diff/1/src/compiler/js-typed-lower...
File src/compiler/js-typed-lowering.cc (right):

https://codereview.chromium.org/2934893002/diff/1/src/compiler/js-typed-lower...
src/compiler/js-typed-lowering.cc:1394:
JSTypedLowering::InferHasInPrototypeChain(Node* receiver, Node* effect,
Moved here from JSNativeContextSpecialization.

https://codereview.chromium.org/2934893002/diff/1/src/compiler/js-typed-lower...
src/compiler/js-typed-lowering.cc:1444: Reduction
JSTypedLowering::ReduceJSHasInPrototypeChain(Node* node) {
Moved here from JSNativeContextSpecialization::ReduceJSOrdinaryHasInstance.

[Benedikt Meurer, 2017-06-12 19:54:47]

Description was changed from

==========
[builtins] Properly optimize Object.prototype.isPrototypeOf.

Port the baseline implementation of Object.prototype.isPrototypeOf to
the CodeStubAssembler, sharing the existing prototype chain lookup logic
with the instanceof / OrdinaryHasInstance implementation. Based on that,
do the same in TurboFan, introducing a new JSHasInPrototypeChain
operator, which encapsulates the central prototype chain walk logic.

This speeds up Object.prototype.isPrototypeOf by more than a factor of
four, so that the code

  A.prototype.isPrototypeOf(a)

is now performance-wise on par with

  a instanceof A

for the case where A is a regular constructor function and a is an
instance of A.

Since instanceof does more than just the fundamental prototype chain
lookup, it was discovered in Node core that O.p.isPrototypeOf would
be a more appropriate alternative for certain sanity checks, since
it's less vulnerable to monkey-patching. In addition, the Object
builtin would also avoid the performance-cliff associated with
instanceof (due to the Symbol.hasInstance hook), as for example hit
by https://github.com/nodejs/node/pull/13403#issuecomment-305915874.
The main blocker was the missing performance of isPrototypeOf, since
it was still a JS builtin backed by a runtime call.

This CL also adds more test coverage for the
Object.prototype.isPrototypeOf builtin, especially when called from
optimized code.

BUG=v8:5269,v8:5989,v8:6483
R=jgruber@chromium.org
==========

to

==========
[builtins] Properly optimize Object.prototype.isPrototypeOf.

Port the baseline implementation of Object.prototype.isPrototypeOf to
the CodeStubAssembler, sharing the existing prototype chain lookup logic
with the instanceof / OrdinaryHasInstance implementation. Based on that,
do the same in TurboFan, introducing a new JSHasInPrototypeChain
operator, which encapsulates the central prototype chain walk logic.

This speeds up Object.prototype.isPrototypeOf by more than a factor of
four, so that the code

  A.prototype.isPrototypeOf(a)

is now performance-wise on par with

  a instanceof A

for the case where A is a regular constructor function and a is an
instance of A.

Since instanceof does more than just the fundamental prototype chain
lookup, it was discovered in Node core that O.p.isPrototypeOf would
be a more appropriate alternative for certain sanity checks, since
it's less vulnerable to monkey-patching. In addition, the Object
builtin would also avoid the performance-cliff associated with
instanceof (due to the Symbol.hasInstance hook), as for example hit
by https://github.com/nodejs/node/pull/13403#issuecomment-305915874.
The main blocker was the missing performance of isPrototypeOf, since
it was still a JS builtin backed by a runtime call.

This CL also adds more test coverage for the
Object.prototype.isPrototypeOf builtin, especially when called from
optimized code.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng
BUG=v8:5269,v8:5989,v8:6483
R=jgruber@chromium.org
==========

[Benedikt Meurer, 2017-06-12 19:54:52]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2017-06-12 19:54:56]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2017-06-12 19:54:59]

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-12 19:55:00]

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

[commit-bot: I haz the power, 2017-06-12 19:55:01]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-v8-committers".
Note that this has nothing to do with OWNERS files.

[Benedikt Meurer, 2017-06-12 19:56:56]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2017-06-12 19:57:07]

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

[commit-bot: I haz the power, 2017-06-12 19:57:18]

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-12 20:10:09]

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

[commit-bot: I haz the power, 2017-06-12 20:10:10]

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

[Benedikt Meurer, 2017-06-12 20:17:21]

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

[commit-bot: I haz the power, 2017-06-12 20:17:31]

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-12 21:29:43]

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

[commit-bot: I haz the power, 2017-06-12 21:29:43]

Dry run: This issue passed the CQ dry run.

[jgruber, 2017-06-13 07:49:52]

lgtm, thanks!

Looked at compiler/ and it looks like the lowering is doing the right thing, but
I don't know that code enough to do a real review of js-typed-lowering.

https://codereview.chromium.org/2934893002/diff/20001/src/builtins/builtins-o...
File src/builtins/builtins-object-gen.cc (right):

https://codereview.chromium.org/2934893002/diff/20001/src/builtins/builtins-o...
src/builtins/builtins-object-gen.cc:219: GotoIf(WordEqual(receiver,
NullConstant()), &if_receiverisnullorundefined);
Nit: IsNull, IsUndefined (predicates are autogenerated for everything in CSA's
HEAP_CONSTANT_LIST).

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

https://codereview.chromium.org/2934893002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:6007: // Loop through the prototype chain looking for
the {callable} prototype.
Nit: comment needs to be updated ({callable}).

https://codereview.chromium.org/2934893002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:6035: GotoIf(WordEqual(object_prototype,
NullConstant()), &return_false);
Nit: IsNull

https://codereview.chromium.org/2934893002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:6039: var_object_map.Bind(LoadMap(object_prototype));
Please CSA_ASSERT(TaggedIsNotSmi(object_prototype)).

https://codereview.chromium.org/2934893002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:6044: var_result.Bind(BooleanConstant(true));
Nit: Here and other spots, FalseConstant / TrueConstant may be simpler.

[Benedikt Meurer, 2017-06-13 18:22:45]

Thanks for the review! Feedback addressed.

https://codereview.chromium.org/2934893002/diff/20001/src/builtins/builtins-o...
File src/builtins/builtins-object-gen.cc (right):

https://codereview.chromium.org/2934893002/diff/20001/src/builtins/builtins-o...
src/builtins/builtins-object-gen.cc:219: GotoIf(WordEqual(receiver,
NullConstant()), &if_receiverisnullorundefined);
On 2017/06/13 07:49:51, jgruber wrote:
> Nit: IsNull, IsUndefined (predicates are autogenerated for everything in CSA's
> HEAP_CONSTANT_LIST).

Done.

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

https://codereview.chromium.org/2934893002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:6007: // Loop through the prototype chain looking for
the {callable} prototype.
On 2017/06/13 07:49:52, jgruber wrote:
> Nit: comment needs to be updated ({callable}).

Done.

https://codereview.chromium.org/2934893002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:6035: GotoIf(WordEqual(object_prototype,
NullConstant()), &return_false);
On 2017/06/13 07:49:52, jgruber wrote:
> Nit: IsNull

Done.

https://codereview.chromium.org/2934893002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:6039: var_object_map.Bind(LoadMap(object_prototype));
On 2017/06/13 07:49:52, jgruber wrote:
> Please CSA_ASSERT(TaggedIsNotSmi(object_prototype)).

Done.

https://codereview.chromium.org/2934893002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:6044: var_result.Bind(BooleanConstant(true));
On 2017/06/13 07:49:52, jgruber wrote:
> Nit: Here and other spots, FalseConstant / TrueConstant may be simpler.

Done.

[Benedikt Meurer, 2017-06-13 18:23:25]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2017-06-13 18:23:27]

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

[commit-bot: I haz the power, 2017-06-13 18:23:35]

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-13 19:14:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497378205948350,
"parent_rev": "8196e102654935eb780c8fa868b97d3409b7c0e6", "commit_rev":
"b11c557d32846fde2615621ac7528e28dd992c86"}

[commit-bot: I haz the power, 2017-06-13 19:14:07]

Description was changed from

==========
[builtins] Properly optimize Object.prototype.isPrototypeOf.

Port the baseline implementation of Object.prototype.isPrototypeOf to
the CodeStubAssembler, sharing the existing prototype chain lookup logic
with the instanceof / OrdinaryHasInstance implementation. Based on that,
do the same in TurboFan, introducing a new JSHasInPrototypeChain
operator, which encapsulates the central prototype chain walk logic.

This speeds up Object.prototype.isPrototypeOf by more than a factor of
four, so that the code

  A.prototype.isPrototypeOf(a)

is now performance-wise on par with

  a instanceof A

for the case where A is a regular constructor function and a is an
instance of A.

Since instanceof does more than just the fundamental prototype chain
lookup, it was discovered in Node core that O.p.isPrototypeOf would
be a more appropriate alternative for certain sanity checks, since
it's less vulnerable to monkey-patching. In addition, the Object
builtin would also avoid the performance-cliff associated with
instanceof (due to the Symbol.hasInstance hook), as for example hit
by https://github.com/nodejs/node/pull/13403#issuecomment-305915874.
The main blocker was the missing performance of isPrototypeOf, since
it was still a JS builtin backed by a runtime call.

This CL also adds more test coverage for the
Object.prototype.isPrototypeOf builtin, especially when called from
optimized code.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng
BUG=v8:5269,v8:5989,v8:6483
R=jgruber@chromium.org
==========

to

==========
[builtins] Properly optimize Object.prototype.isPrototypeOf.

Port the baseline implementation of Object.prototype.isPrototypeOf to
the CodeStubAssembler, sharing the existing prototype chain lookup logic
with the instanceof / OrdinaryHasInstance implementation. Based on that,
do the same in TurboFan, introducing a new JSHasInPrototypeChain
operator, which encapsulates the central prototype chain walk logic.

This speeds up Object.prototype.isPrototypeOf by more than a factor of
four, so that the code

  A.prototype.isPrototypeOf(a)

is now performance-wise on par with

  a instanceof A

for the case where A is a regular constructor function and a is an
instance of A.

Since instanceof does more than just the fundamental prototype chain
lookup, it was discovered in Node core that O.p.isPrototypeOf would
be a more appropriate alternative for certain sanity checks, since
it's less vulnerable to monkey-patching. In addition, the Object
builtin would also avoid the performance-cliff associated with
instanceof (due to the Symbol.hasInstance hook), as for example hit
by https://github.com/nodejs/node/pull/13403#issuecomment-305915874.
The main blocker was the missing performance of isPrototypeOf, since
it was still a JS builtin backed by a runtime call.

This CL also adds more test coverage for the
Object.prototype.isPrototypeOf builtin, especially when called from
optimized code.

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng
BUG=v8:5269,v8:5989,v8:6483
R=jgruber@chromium.org

Review-Url: https://codereview.chromium.org/2934893002
Cr-Commit-Position: refs/heads/master@{#45925}
Committed:
https://chromium.googlesource.com/v8/v8/+/b11c557d32846fde2615621ac7528e28dd9...
==========

[commit-bot: I haz the power, 2017-06-13 19:14:09]

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