[js] Avoid %_ClassOf for collection builtins.

[js] Avoid %_ClassOf for collection builtins.

The collection builtins (Map, Set, WeakMap, WeakSet) are still written
in JavaScript and make heavy use of %_ClassOf, which is kind of
expensive compared to a simple instance type check. Change that to use
simple instance type checks instead.

R=jarin@chromium.org
BUG=v8:6261, v8:6278, v8:6344
Review-Url: https://codereview.chromium.org/2814773005
Cr-Original-Commit-Position: refs/heads/master@{#45106}
Committed: https://chromium.googlesource.com/v8/v8/+/28170099fd1efc84a724ef133f335fec521c0852
Review-Url: https://codereview.chromium.org/2814773005
Cr-Commit-Position: refs/heads/master@{#45124}
Committed: https://chromium.googlesource.com/v8/v8/+/516d8438adc44bfab2d99e90d23532fc92dfff1f

[Benedikt Meurer, 2017-04-13 11:18:18]

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

[commit-bot: I haz the power, 2017-04-13 11:18:26]

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

[Jarin, 2017-04-13 11:20:31]

lgtm

[Benedikt Meurer, 2017-04-13 11:20:40]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2017-04-13 11:20:45]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2017-04-13 11:20:47]

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 11:39:46]

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

[commit-bot: I haz the power, 2017-04-13 11:39:47]

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-04-13 12:03:15]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2017-04-13 12:03:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org
Link to the patchset: https://codereview.chromium.org/2814773005/#ps20001
(title: "Make debug-evaluate happy.")

[commit-bot: I haz the power, 2017-04-13 12:03:22]

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 12:23:50]

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

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

Try jobs failed on following builders:
  v8_linux64_verify_csa_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)
  v8_linux64_verify_csa_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_verify_csa_rel_n...)

[adamk, 2017-05-02 22:09:40]

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

[adamk, 2017-05-02 22:09:41]

Is this ready to land? I was about to write up something similar myself but then
remembered I saw this fly by recently.

lgtm fwiw :)

[Benedikt Meurer, 2017-05-03 02:06:49]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2017-05-03 02:06: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-05-03 02:08:28]

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

[commit-bot: I haz the power, 2017-05-03 02:08:29]

Try jobs failed on following builders:
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/25270)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/40315)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)

[Benedikt Meurer, 2017-05-03 02:48:09]

On 2017/05/02 22:09:41, adamk wrote:
> Is this ready to land? I was about to write up something similar myself but
then
> remembered I saw this fly by recently.
> 
> lgtm fwiw :)

I have no idea about the test failure in command line-out, feel free to take
over this cl.

[adamk, 2017-05-03 22:54:31]

On 2017/05/03 02:48:09, Benedikt Meurer wrote:
> On 2017/05/02 22:09:41, adamk wrote:
> > Is this ready to land? I was about to write up something similar myself but
> then
> > remembered I saw this fly by recently.
> > 
> > lgtm fwiw :)
> 
> I have no idea about the test failure in command line-out, feel free to take
> over this cl.

This is some bad behavior in Crankshaft, where it's outputting a SourcePosition
for a deopt point with no actual source position. Not sure how worthwhile it is
to try to debug this further given that it only triggers with --no-turbo.

[adamk, 2017-05-03 23:01:35]

On 2017/05/03 22:54:31, adamk wrote:
> On 2017/05/03 02:48:09, Benedikt Meurer wrote:
> > On 2017/05/02 22:09:41, adamk wrote:
> > > Is this ready to land? I was about to write up something similar myself
but
> > then
> > > remembered I saw this fly by recently.
> > > 
> > > lgtm fwiw :)
> > 
> > I have no idea about the test failure in command line-out, feel free to take
> > over this cl.
> 
> This is some bad behavior in Crankshaft, where it's outputting a
SourcePosition
> for a deopt point with no actual source position. Not sure how worthwhile it
is
> to try to debug this further given that it only triggers with --no-turbo.

Actually, I'm not entirely certain this is Crankshaft-specific, it seems like TF
simply doesn't try to optimize Set.prototype.add.

Even more strangely, the bad deopt info appears to be coming from the generation
of code for ToBooleanICStub (which happens to be a HydrogenCodeStub).

[adamk, 2017-05-04 17:31:28]

https://codereview.chromium.org/2814773005/diff/20001/src/compiler/js-intrins...
File src/compiler/js-intrinsic-lowering.cc (right):

https://codereview.chromium.org/2814773005/diff/20001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:57: case Runtime::kInlineIsJSGlobalProxy:
You can also add these to the no-FrameState whitelist in linkage.cc

[Benedikt Meurer, 2017-05-04 17:50:40]

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

[commit-bot: I haz the power, 2017-05-04 17:50:51]

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-05-04 17:50:53]

https://codereview.chromium.org/2814773005/diff/20001/src/compiler/js-intrins...
File src/compiler/js-intrinsic-lowering.cc (right):

https://codereview.chromium.org/2814773005/diff/20001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:57: case Runtime::kInlineIsJSGlobalProxy:
Good catch, done!

[Benedikt Meurer, 2017-05-04 17:59:37]

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

[commit-bot: I haz the power, 2017-05-04 17:59:42]

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-05-04 17:59:50]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2017-05-04 17:59:54]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2017-05-04 17:59:54]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org,
adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2814773005/#ps60001
(title: "Skip failing inspector test in noturbofan variant (issue related to
missing source position in ToBo…")

[commit-bot: I haz the power, 2017-05-04 18:00:05]

CQ is trying da patch.

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

[Benedikt Meurer, 2017-05-04 18:02:04]

Description was changed from

==========
[js] Avoid %_ClassOf for collection builtins.

The collection builtins (Map, Set, WeakMap, WeakSet) are still written
in JavaScript and make heavy use of %_ClassOf, which is kind of
expensive compared to a simple instance type check. Change that to use
simple instance type checks instead.

R=jarin@chromium.org
BUG=v8:6261
==========

to

==========
[js] Avoid %_ClassOf for collection builtins.

The collection builtins (Map, Set, WeakMap, WeakSet) are still written
in JavaScript and make heavy use of %_ClassOf, which is kind of
expensive compared to a simple instance type check. Change that to use
simple instance type checks instead.

R=jarin@chromium.org
BUG=v8:6261,v8:6278,v8:6344
==========

[commit-bot: I haz the power, 2017-05-04 18:27:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493920794475600,
"parent_rev": "82503e9ba30f38d428431f69a82f885569ac4913", "commit_rev":
"28170099fd1efc84a724ef133f335fec521c0852"}

[commit-bot: I haz the power, 2017-05-04 18:27:43]

Description was changed from

==========
[js] Avoid %_ClassOf for collection builtins.

The collection builtins (Map, Set, WeakMap, WeakSet) are still written
in JavaScript and make heavy use of %_ClassOf, which is kind of
expensive compared to a simple instance type check. Change that to use
simple instance type checks instead.

R=jarin@chromium.org
BUG=v8:6261,v8:6278,v8:6344
==========

to

==========
[js] Avoid %_ClassOf for collection builtins.

The collection builtins (Map, Set, WeakMap, WeakSet) are still written
in JavaScript and make heavy use of %_ClassOf, which is kind of
expensive compared to a simple instance type check. Change that to use
simple instance type checks instead.

R=jarin@chromium.org
BUG=v8:6261,v8:6278,v8:6344

Review-Url: https://codereview.chromium.org/2814773005
Cr-Commit-Position: refs/heads/master@{#45106}
Committed:
https://chromium.googlesource.com/v8/v8/+/28170099fd1efc84a724ef133f335fec521...
==========

[commit-bot: I haz the power, 2017-05-04 18:27:45]

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

[Benedikt Meurer, 2017-05-04 18:43:22]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2860123002/ by bmeurer@chromium.org.

The reason for reverting is: Breaks node.js integration bot:
https://build.chromium.org/p/client.v8.fyi/builders/V8%20-%20node.js%20integr....

[Benedikt Meurer, 2017-05-05 07:27:14]

Description was changed from

==========
[js] Avoid %_ClassOf for collection builtins.

The collection builtins (Map, Set, WeakMap, WeakSet) are still written
in JavaScript and make heavy use of %_ClassOf, which is kind of
expensive compared to a simple instance type check. Change that to use
simple instance type checks instead.

R=jarin@chromium.org
BUG=v8:6261,v8:6278,v8:6344

Review-Url: https://codereview.chromium.org/2814773005
Cr-Commit-Position: refs/heads/master@{#45106}
Committed:
https://chromium.googlesource.com/v8/v8/+/28170099fd1efc84a724ef133f335fec521...
==========

to

==========
[js] Avoid %_ClassOf for collection builtins.

The collection builtins (Map, Set, WeakMap, WeakSet) are still written
in JavaScript and make heavy use of %_ClassOf, which is kind of
expensive compared to a simple instance type check. Change that to use
simple instance type checks instead.

R=jarin@chromium.org
BUG=v8:6261,v8:6278,v8:6344

Review-Url: https://codereview.chromium.org/2814773005
Cr-Commit-Position: refs/heads/master@{#45106}
Committed:
https://chromium.googlesource.com/v8/v8/+/28170099fd1efc84a724ef133f335fec521...
==========

[Benedikt Meurer, 2017-05-05 09:14:14]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2017-05-05 09:14:14]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org,
adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2814773005/#ps80001
(title: "Remove the IsJSGlobalProxy magic.")

[commit-bot: I haz the power, 2017-05-05 09:14:24]

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-05-05 09:41:20]

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

[commit-bot: I haz the power, 2017-05-05 09:41:21]

Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/25515)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[Benedikt Meurer, 2017-05-05 09:52:43]

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

[commit-bot: I haz the power, 2017-05-05 09:52:50]

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-05-05 09:53:31]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2017-05-05 09:53:44]

The CQ bit was checked by bmeurer@chromium.org

[Benedikt Meurer, 2017-05-05 09:53:44]

The patchset sent to the CQ was uploaded after l-g-t-m from jarin@chromium.org,
adamk@chromium.org
Link to the patchset: https://codereview.chromium.org/2814773005/#ps100001
(title: "Disable the failing test for CS again.")

[commit-bot: I haz the power, 2017-05-05 09:53: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-05-05 10:22:18]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493978024239150,
"parent_rev": "c0a65cd295301f334e2bbfcce46a937cd708beec", "commit_rev":
"516d8438adc44bfab2d99e90d23532fc92dfff1f"}

[commit-bot: I haz the power, 2017-05-05 10:22:26]

Description was changed from

==========
[js] Avoid %_ClassOf for collection builtins.

The collection builtins (Map, Set, WeakMap, WeakSet) are still written
in JavaScript and make heavy use of %_ClassOf, which is kind of
expensive compared to a simple instance type check. Change that to use
simple instance type checks instead.

R=jarin@chromium.org
BUG=v8:6261,v8:6278,v8:6344

Review-Url: https://codereview.chromium.org/2814773005
Cr-Commit-Position: refs/heads/master@{#45106}
Committed:
https://chromium.googlesource.com/v8/v8/+/28170099fd1efc84a724ef133f335fec521...
==========

to

==========
[js] Avoid %_ClassOf for collection builtins.

The collection builtins (Map, Set, WeakMap, WeakSet) are still written
in JavaScript and make heavy use of %_ClassOf, which is kind of
expensive compared to a simple instance type check. Change that to use
simple instance type checks instead.

R=jarin@chromium.org
BUG=v8:6261,v8:6278,v8:6344

Review-Url: https://codereview.chromium.org/2814773005
Cr-Original-Commit-Position: refs/heads/master@{#45106}
Committed:
https://chromium.googlesource.com/v8/v8/+/28170099fd1efc84a724ef133f335fec521...
Review-Url: https://codereview.chromium.org/2814773005
Cr-Commit-Position: refs/heads/master@{#45124}
Committed:
https://chromium.googlesource.com/v8/v8/+/516d8438adc44bfab2d99e90d23532fc92d...
==========

[commit-bot: I haz the power, 2017-05-05 10:22:27]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/v8/v8/+/516d8438adc44bfab2d99e90d23532fc92d...

[rmcilroy, 2017-05-05 13:45:30]

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

[rmcilroy, 2017-05-05 13:45:31]

https://codereview.chromium.org/2814773005/diff/100001/src/compiler/js-intrin...
File src/compiler/js-intrinsic-lowering.cc (right):

https://codereview.chromium.org/2814773005/diff/100001/src/compiler/js-intrin...
src/compiler/js-intrinsic-lowering.cc:70: return ReduceIsInstanceType(node,
JS_WEAK_SET_TYPE);
Does it make sense to add these as intrinsics to the interpreter as well? We
currently call out to the runtime for all these that you added here.

[Benedikt Meurer, 2017-05-05 14:00:58]

https://codereview.chromium.org/2814773005/diff/100001/src/compiler/js-intrin...
File src/compiler/js-intrinsic-lowering.cc (right):

https://codereview.chromium.org/2814773005/diff/100001/src/compiler/js-intrin...
src/compiler/js-intrinsic-lowering.cc:70: return ReduceIsInstanceType(node,
JS_WEAK_SET_TYPE);
Ideally this code should be obsolete soon with a CSA based Maps/Sets. Maybe
until then, handling it in the interpreter might be beneficial indeed.

[adamk, 2017-05-17 19:22:31]

On 2017/05/04 18:43:22, Benedikt Meurer wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/2860123002/ by mailto:bmeurer@chromium.org.
> 
> The reason for reverting is: Breaks node.js integration bot:
>
https://build.chromium.org/p/client.v8.fyi/builders/V8%20-%20node.js%20integr....

What was the particular breakage here? It looks to me like the overhead of the
%_ClassOf check for IS_GLOBAL is still noticeable in JS version of
GetExistingHash() when called on a JSReceiver.

[Benedikt Meurer, 2017-05-17 19:23:55]

On 2017/05/17 19:22:31, adamk wrote:
> On 2017/05/04 18:43:22, Benedikt Meurer wrote:
> > A revert of this CL (patchset #4 id:60001) has been created in
> > https://codereview.chromium.org/2860123002/ by mailto:bmeurer@chromium.org.
> > 
> > The reason for reverting is: Breaks node.js integration bot:
> >
>
https://build.chromium.org/p/client.v8.fyi/builders/V8%20-%20node.js%20integr....
> 
> What was the particular breakage here? It looks to me like the overhead of the
> %_ClassOf check for IS_GLOBAL is still noticeable in JS version of
> GetExistingHash() when called on a JSReceiver.

It was failing for bizarre reasons that I couldn't understand. I didn't spend a
lot of time investigating, so it might be possible to make it work.