[Interpreter] Transform StrictEquality with null/undefined to special bytecodes.

[Interpreter] Transform StrictEquality with null/undefined to special bytecodes.

Transform LdaNull/LdaUndefined followed by StrictEquality to TestNull/TestUndefined. 
This would avoid a call to the compare IC. In the bytecode-graph builder these are
mapped to StrictEqual javascript operator. When reducing this operator, we already
optimize the cases for null/undefined.

BUG=v8:4280
Review-Url: https://codereview.chromium.org/2554723004
Cr-Commit-Position: refs/heads/master@{#41768}
Committed: https://chromium.googlesource.com/v8/v8/+/74ccda64dc6b3ecec1eecf8987da37a106da57de

[mythria, 2016-12-08 10:50:27]

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

[commit-bot: I haz the power, 2016-12-08 10:50:32]

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, 2016-12-08 11:19:00]

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

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

Dry run: This issue passed the CQ dry run.

[mythria, 2016-12-12 11:26:37]

Description was changed from

==========
[Interpreter] Transform StrictEquality with null/undefined to special bytecodes.

Transform LdaNull/LdaUndefined followed by StrictEquality to
TestNull/TestUndefined.

BUG=v8:4280
==========

to

==========
[Interpreter] Transform StrictEquality with null/undefined to special bytecodes.

Transform LdaNull/LdaUndefined followed by StrictEquality to
TestNull/TestUndefined. 
This would avoid a call to the compare IC. In the bytecode-graph builder these
are mapped to
StrictEqual javascript operator. When reducing this operator, we already
optimize the cases
for null/undefined.

BUG=v8:4280
==========

[mythria, 2016-12-12 11:26:53]

Description was changed from

==========
[Interpreter] Transform StrictEquality with null/undefined to special bytecodes.

Transform LdaNull/LdaUndefined followed by StrictEquality to
TestNull/TestUndefined. 
This would avoid a call to the compare IC. In the bytecode-graph builder these
are mapped to
StrictEqual javascript operator. When reducing this operator, we already
optimize the cases
for null/undefined.

BUG=v8:4280
==========

to

==========
[Interpreter] Transform StrictEquality with null/undefined to special bytecodes.

Transform LdaNull/LdaUndefined followed by StrictEquality to
TestNull/TestUndefined. 
This would avoid a call to the compare IC. In the bytecode-graph builder these
are mapped to
StrictEqual javascript operator. When reducing this operator, we already
optimize the cases
for null/undefined.

BUG=v8:4280
==========

[mythria, 2016-12-12 11:27:13]

Description was changed from

==========
[Interpreter] Transform StrictEquality with null/undefined to special bytecodes.

Transform LdaNull/LdaUndefined followed by StrictEquality to
TestNull/TestUndefined. 
This would avoid a call to the compare IC. In the bytecode-graph builder these
are mapped to
StrictEqual javascript operator. When reducing this operator, we already
optimize the cases
for null/undefined.

BUG=v8:4280
==========

to

==========
[Interpreter] Transform StrictEquality with null/undefined to special bytecodes.

Transform LdaNull/LdaUndefined followed by StrictEquality to
TestNull/TestUndefined. 
This would avoid a call to the compare IC. In the bytecode-graph builder these
are
mapped to StrictEqual javascript operator. When reducing this operator, we
already
optimize the cases for null/undefined.

BUG=v8:4280
==========

[mythria, 2016-12-12 11:36:04]

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

[mythria, 2016-12-12 11:36:05]

This cl is to optimize strict equals check with null or undefined. On benchmarks
it does not have any impact. In the real world pages, it seems to help cnn.com.
With cnn.com, it seems to improve on all three times I tried. There are few more
improvements but not consistent and in different groups which I don't think
should be impacted.

It has a quite strange behaviour on adwords. It seems to impact it by +/-10%.
10% is like almost a second in the the actual time. I am not quite sure, I tried
to look at it but it is not consistent. I thought may be we could land this and
revert it back if it regresses anything. 

PTAL.

Thanks,
Mythri

[rmcilroy, 2016-12-14 18:50:00]

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

[rmcilroy, 2016-12-14 18:50:00]

+Benedikt

High level question - is it worth having three bytecodes for these cases (along
with the undetectable bytecode before)? Another option would be to have a
TestOddball which has an operand to specify which oddball we want to check
against.

Also Mythri, appologies I didn't look at this earlier, but I thought this was a
CL to look at fusing typeof and strict/abstract equality (as mentioned in
Beneditk's doc) - that would actually probably have more impact since people do
"typeof(x) === “string” / “number” / “function” in real world code. Maybe you
could have a look at this when you get a chance?

https://codereview.chromium.org/2554723004/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2554723004/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1955: // Test if the value in the <src> register
is strictly equal to Null.
nit Null -> null

https://codereview.chromium.org/2554723004/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1978: // Test if the value in the <src> register
is strictly equal to Undefined.
nit - Undefined -> undefined.

[Benedikt Meurer, 2016-12-14 18:56:22]

On 2016/12/14 18:50:00, rmcilroy wrote:
> +Benedikt
> 
> High level question - is it worth having three bytecodes for these cases
(along
> with the undetectable bytecode before)? Another option would be to have a
> TestOddball which has an operand to specify which oddball we want to check
> against.

I think explicit bytecodes are better and easier to debug too. We can always
compress bytecode space later if necessary.

[mythria, 2016-12-15 09:27:39]

Thanks Benedikt and Ross. I fixed the nits.

I will also look at the typeof optimization today. 

Thanks,
Mythri

https://codereview.chromium.org/2554723004/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/2554723004/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1955: // Test if the value in the <src> register
is strictly equal to Null.
On 2016/12/14 18:50:00, rmcilroy wrote:
> nit Null -> null

Done.

https://codereview.chromium.org/2554723004/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1978: // Test if the value in the <src> register
is strictly equal to Undefined.
On 2016/12/14 18:50:00, rmcilroy wrote:
> nit - Undefined -> undefined.

Done.

[mythria, 2016-12-15 09:36:24]

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

[commit-bot: I haz the power, 2016-12-15 09:36:32]

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, 2016-12-15 09:38:02]

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

[commit-bot: I haz the power, 2016-12-15 09:38:03]

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/17829)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  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/17803)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30632)
  v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/19424)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/19538)

[mythria, 2016-12-15 10:10:49]

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

[commit-bot: I haz the power, 2016-12-15 10:10:52]

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, 2016-12-15 10:36:47]

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

[commit-bot: I haz the power, 2016-12-15 10:36:48]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-12-15 12:11:47]

LGTM, thanks.

[mythria, 2016-12-15 14:16:12]

The CQ bit was checked by mythria@chromium.org

[commit-bot: I haz the power, 2016-12-15 14:16: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, 2016-12-15 14:19:13]

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

[commit-bot: I haz the power, 2016-12-15 14:19:14]

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

[mythria, 2016-12-16 08:58:24]

Benedikt, could PTAL at compiler changes. 

Thanks,
Mythri

[Benedikt Meurer, 2016-12-16 09:16:09]

lgtm

[mythria, 2016-12-16 14:33:10]

The CQ bit was checked by mythria@chromium.org

[commit-bot: I haz the power, 2016-12-16 14:33:14]

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-16 15:01:09]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481898790200250,
"parent_rev": "692ba84f4fa30df106129d6f376448b4b421e892", "commit_rev":
"74ccda64dc6b3ecec1eecf8987da37a106da57de"}

[commit-bot: I haz the power, 2016-12-16 15:01:15]

Description was changed from

==========
[Interpreter] Transform StrictEquality with null/undefined to special bytecodes.

Transform LdaNull/LdaUndefined followed by StrictEquality to
TestNull/TestUndefined. 
This would avoid a call to the compare IC. In the bytecode-graph builder these
are
mapped to StrictEqual javascript operator. When reducing this operator, we
already
optimize the cases for null/undefined.

BUG=v8:4280
==========

to

==========
[Interpreter] Transform StrictEquality with null/undefined to special bytecodes.

Transform LdaNull/LdaUndefined followed by StrictEquality to
TestNull/TestUndefined. 
This would avoid a call to the compare IC. In the bytecode-graph builder these
are
mapped to StrictEqual javascript operator. When reducing this operator, we
already
optimize the cases for null/undefined.

BUG=v8:4280

Review-Url: https://codereview.chromium.org/2554723004
Cr-Commit-Position: refs/heads/master@{#41768}
Committed:
https://chromium.googlesource.com/v8/v8/+/74ccda64dc6b3ecec1eecf8987da37a106d...
==========

[commit-bot: I haz the power, 2016-12-16 15:01:16]

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

[Benedikt Meurer, 2016-12-16 20:18:36]

This gave a nice improvement on Box2D with I+TF!

[Jarin, 2016-12-16 20:51:34]

On 2016/12/16 20:18:36, Benedikt Meurer wrote:
> This gave a nice improvement on Box2D with I+TF!

Niiiice. Any theory why it helps so much? In the optimizing compiler, the graph
should be the same, no?

Is it because we spend so much time in the interpreter? (That would be
worrying...)

[Benedikt Meurer, 2016-12-16 21:09:43]

On 2016/12/16 20:51:34, Jarin wrote:
> On 2016/12/16 20:18:36, Benedikt Meurer wrote:
> > This gave a nice improvement on Box2D with I+TF!
> 
> Niiiice. Any theory why it helps so much? In the optimizing compiler, the
graph
> should be the same, no?
> 
> Is it because we spend so much time in the interpreter? (That would be
> worrying...)

It does indeed look like we spend incredible amount of time in Ignition. Maybe
there's still something wrong with the heuristics for the full pipeline (that'd
also explain a couple of other tankages we saw).

[rmcilroy, 2016-12-16 23:43:01]

On 2016/12/16 21:09:43, Benedikt Meurer wrote:
> On 2016/12/16 20:51:34, Jarin wrote:
> > On 2016/12/16 20:18:36, Benedikt Meurer wrote:
> > > This gave a nice improvement on Box2D with I+TF!
> > 
> > Niiiice. Any theory why it helps so much? In the optimizing compiler, the
> graph
> > should be the same, no?
> > 
> > Is it because we spend so much time in the interpreter? (That would be
> > worrying...)
> 
> It does indeed look like we spend incredible amount of time in Ignition. Maybe
> there's still something wrong with the heuristics for the full pipeline
(that'd
> also explain a couple of other tankages we saw).

Nice to see the improvement in I+TF.  However, I don't think the this is due to
us spending too much time in Ignition though - this CL doesn't impact the score
of Box2D for Ignition only (TF+CS+FCG disabled) at all [1], so I don't see how
the I+TF improvement could be down to Ignition. As Jaro says though, the TF
graph should be the same, so I'm not sure what's going on. The only other thing
I can think of is maybe we have an issue with our type collection / usage which
got bypassed with this change - any thoughts?

 [1]
https://chromeperf.appspot.com/report?sid=4befbc91195b5ae9b19efccb8d037f1e3ed...)
in_reply_to: 5688737870643200

[rmcilroy, 2016-12-17 13:00:54]

I have a theory as to why we see the I+TF improvement here, I added Mythri and
Jaro to a thread where I describe what might be going on.