Introduce IsUndefined(Isolate*) and IsTheHole(Isolate*)

Introduce IsUndefined(Isolate*) and IsTheHole(Isolate*)

Passing in the isolate and pointer compare the instnance against the
corresponding constant is always faster than decoding the instance types.

BUG=

Committed: https://crrev.com/bc0798ca1a7e28eb82f03f40184b3b252f389fc4
Cr-Commit-Position: refs/heads/master@{#36744}

[Camillo Bruni, 2016-06-01 08:23:41]

cbruni@chromium.org changed reviewers:
+ ulan@chromium.org, verwaest@chromium.org

[Camillo Bruni, 2016-06-01 08:23:42]

ula@ PTAL heap/*
verwaest@ PTAL the rest.

[ulan, 2016-06-01 08:35:58]

Is the performance difference measurable?

In the old implementation we do two loads obj->map()->instance_type() and bit
twiddling.

In the new implementation we also do two loads isolate->heap()->undefined().

Does the difference come heap() and undefined() being CPU cache?

[Yang, 2016-06-01 08:39:24]

On 2016/06/01 08:35:58, ulan wrote:
> Is the performance difference measurable?
> 
> In the old implementation we do two loads obj->map()->instance_type() and bit
> twiddling.
> 
> In the new implementation we also do two loads isolate->heap()->undefined().
> 
> Does the difference come heap() and undefined() being CPU cache?

isolate->heap() is just offset calculation though. Heap is a member of isolate.

[Camillo Bruni, 2016-06-01 10:31:27]

On 2016/06/01 at 08:39:24, yangguo wrote:
> On 2016/06/01 08:35:58, ulan wrote:
> > Is the performance difference measurable?
> > 
> > In the old implementation we do two loads obj->map()->instance_type() and
bit
> > twiddling.
> > 
> > In the new implementation we also do two loads isolate->heap()->undefined().
> > 
> > Does the difference come heap() and undefined() being CPU cache?
> 
> isolate->heap() is just offset calculation though. Heap is a member of
isolate.

Yeah, so Toon noticed this in a tight loop a while ago.
I reran some micro benchmark on holey arrays counting how many holes there are
in there.
- If there are only holes IsTheHole() (IsOddball + type check) is twice as slow
as IsTheHole(isolate)
- for non-holes the new version a bit more than 20% faster

Even doing GetIsolate on every array member was still faster in this case

[ulan, 2016-06-01 10:52:44]

Thanks. Heap part lgtm.

[Toon Verwaest, 2016-06-01 12:36:17]

Thanks, LGTM (with a comment for another CL)
Also we should migrate places where we manually compare already to these new
helpers.

https://codereview.chromium.org/2028983002/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2028983002/diff/20001/src/objects-inl.h#newco...
src/objects-inl.h:179: }
We should replace the versions above with these versions to avoid having to go
over the entire codebase again later...

[Yang, 2016-06-01 12:46:01]

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

[Yang, 2016-06-01 12:46:02]

LGTM.

Also agree with Toon's comment. We've got tons of places where we manually do
the pointer compare.

https://codereview.chromium.org/2028983002/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2028983002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:2104: if (!callback->IsUndefined(isolate_) &&
!callback->IsNull()) {
What about IsNull(Isolate*) :P

[Camillo Bruni, 2016-06-01 15:07:33]

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

[commit-bot: I haz the power, 2016-06-01 15:07:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028983002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2028983002/60001

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

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

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

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

[Camillo Bruni, 2016-06-01 17:04:05]

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

[commit-bot: I haz the power, 2016-06-01 17:04:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028983002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2028983002/100001

[commit-bot: I haz the power, 2016-06-01 17:34:02]

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

[commit-bot: I haz the power, 2016-06-01 17:34:03]

Dry run: This issue passed the CQ dry run.

[ulan, 2016-06-01 19:01:44]

heap changes still look good, but I think we are going to regress hash table
functions with the latest patch.

https://codereview.chromium.org/2028983002/diff/100001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2028983002/diff/100001/src/objects-inl.h#newc...
src/objects-inl.h:3078: if (element->IsUndefined(isolate)) break;
Isn't this performance critical?

Before it was one compare instruction, now it is 4 loads.

https://codereview.chromium.org/2028983002/diff/100001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2028983002/diff/100001/src/objects.h#newcode3879
src/objects.h:3879: if (hash->IsUndefined(GetIsolate())) return kNotFound;
Before it was doing 2 loads
hash->map->instance_type

Now it is doing 4 loads
hash->page->heap->isolate->heap->undefined_value

[Yang, 2016-06-02 04:33:44]

On 2016/06/01 19:01:44, ulan wrote:
> heap changes still look good, but I think we are going to regress hash table
> functions with the latest patch.
> 
> https://codereview.chromium.org/2028983002/diff/100001/src/objects-inl.h
> File src/objects-inl.h (right):
> 
>
https://codereview.chromium.org/2028983002/diff/100001/src/objects-inl.h#newc...
> src/objects-inl.h:3078: if (element->IsUndefined(isolate)) break;
> Isn't this performance critical?
> 
> Before it was one compare instruction, now it is 4 loads.
> 
> https://codereview.chromium.org/2028983002/diff/100001/src/objects.h
> File src/objects.h (right):
> 
>
https://codereview.chromium.org/2028983002/diff/100001/src/objects.h#newcode3879
> src/objects.h:3879: if (hash->IsUndefined(GetIsolate())) return kNotFound;
> Before it was doing 2 loads
> hash->map->instance_type
> 
> Now it is doing 4 loads
> hash->page->heap->isolate->heap->undefined_value

heap->isolate->heap is a no-op. Heap is a member of isolate. We get from the
heap to isolate by hackily subtracting the member offset of the heap in isolate,
and from isolate to heap by adding the member offset. The compiler should be
able to optimize that away. So it's really just
hash->page->heap->undefined_value()

However I agree that this is probably less performant still.

[Camillo Bruni, 2016-06-02 09:13:00]

> heap->isolate->heap is a no-op. Heap is a member of isolate. We get from the
heap to isolate by hackily subtracting the member offset of the heap in isolate,
and from isolate to heap by adding the member offset. The compiler should be
able to optimize that away. So it's really just
hash->page->heap->undefined_value()
> 
> However I agree that this is probably less performant still.

I ran again a little benchmark with the following C++ code:
===========================================================

NO_INLINE(static bool helper1(HeapObject* el)) {
  return el->IsUndefined();
}

NO_INLINE(static bool helper2(HeapObject* el)) {
  return el->IsUndefined(el->GetIsolate());
}

RUNTIME_FUNCTION(Runtime_GetAndResetRuntimeCallStats) {
  HandleScope scope(isolate);
  CONVERT_ARG_HANDLE_CHECKED(JSArray, array, 0);
  CONVERT_ARG_HANDLE_CHECKED(Object, object, 1);

  FixedArray* elements = FixedArray::cast(array->elements());
  int length = Smi::cast(array->length())->value();
  int count = 0;
  if (object->IsTrue()) {
    for (int i =0; i<10000; i++) {
      for (int j = 0; j < length; j++) {
        HeapObject* el = HeapObject::cast(elements->get(j));
        if (helper1(el)) count++;
      }
    }
  } else {
    for (int i =0; i<10000; i++) {
      for (int j = 0; j < length; j++) {
        HeapObject* el = HeapObject::cast(elements->get(j));
        if (helper2(el)) count++;
      }
    }
  }
  return *isolate->factory()->NewHeapNumber(count);
}
===========================================================

And the corresponding JS benchmark to measure performance:

===========================================================
var a = new Array(10000);
var obj = {};
for (var i =0; i<a.length; i++) {
  a[i] = obj;
}

var time;

time = performance.now();
(%GetAndResetRuntimeCallStats(a, true));
var t1 = (performance.now() - time);

time = performance.now();
(%GetAndResetRuntimeCallStats(a, false));
var t2 = (performance.now() - time);

print(t1/t2)
===========================================================

Initializing the Array fully with undefined yields:
  runs = 100
  avg = 1.001800584276224
  geomean = 1.001614049532537
  result = 1.002 +/- 0.019(1.94%)

Initializing the Array fully with an object:
  runs = 100
  avg = 1.1493797475960421
  geomean = 1.1490380573139711
  result = 1.149 +/- 0.028(2.45%)

So it seems there is almost no benefit of the "fast" path on x64.

[Camillo Bruni, 2016-06-02 10:56:44]

On 2016/06/02 at 09:13:00, Camillo Bruni wrote:

As discussed offline, I will change back the cases where we have explicit
comparisons against undefine and the_hole as there is no real benefit of using
the helper

[Camillo Bruni, 2016-06-02 11:48:36]

PTAL again... :D

https://codereview.chromium.org/2028983002/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2028983002/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:2104: if (!callback->IsUndefined(isolate_) &&
!callback->IsNull()) {
On 2016/06/01 at 12:46:02, Yang wrote:
> What about IsNull(Isolate*) :P

It's on my TODO :)

[ulan, 2016-06-03 08:51:24]

lgtm, thank you

[Camillo Bruni, 2016-06-03 09:41:38]

On 2016/06/03 at 08:51:24, ulan wrote:
> lgtm, thank you

thanks for spending your time reviewing this tedious CL again :)

[Camillo Bruni, 2016-06-03 12:46:51]

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

[commit-bot: I haz the power, 2016-06-03 12:47:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028983002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2028983002/160001

[commit-bot: I haz the power, 2016-06-03 13:16:25]

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

[commit-bot: I haz the power, 2016-06-03 13:16:27]

Dry run: This issue passed the CQ dry run.

[Camillo Bruni, 2016-06-03 13:28:51]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-06-03 13:28:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org, yangguo@chromium.org, ulan@chromium.org
Link to the patchset: https://codereview.chromium.org/2028983002/#ps160001
(title: "doing proper rebase")

[commit-bot: I haz the power, 2016-06-03 13:28:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028983002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2028983002/160001

[commit-bot: I haz the power, 2016-06-03 13:33:43]

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

[commit-bot: I haz the power, 2016-06-03 13:33:45]

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

[Camillo Bruni, 2016-06-03 13:38:26]

cbruni@chromium.org changed reviewers:
+ ahaas@chromium.org, mstarzinger@chromium.org

[Camillo Bruni, 2016-06-03 13:38:27]

mstarzinger@ PTAL (ast/.*)|(compiler/.*)|(interpreter/.*)
ahaas@ PTAL wasm/.*

[ahaas, 2016-06-03 13:45:00]

lgtm

[Michael Starzinger, 2016-06-06 08:55:08]

LGTM with comments.

https://codereview.chromium.org/2028983002/diff/160001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/2028983002/diff/160001/src/compiler/pipeline....
src/compiler/pipeline.cc:537: Isolate* isolate = script->GetIsolate();
nit: info->isolate()

https://codereview.chromium.org/2028983002/diff/160001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2028983002/diff/160001/src/factory.cc#newcode...
src/factory.cc:1305: if (prototype->IsTheHole(code->GetIsolate())) {
nit: isolate()

https://codereview.chromium.org/2028983002/diff/160001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/2028983002/diff/160001/src/frames.cc#newcode1351
src/frames.cc:1351: Isolate* isolate = deopt_data->GetIsolate();
nit: isolate()

https://codereview.chromium.org/2028983002/diff/160001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2028983002/diff/160001/src/objects-inl.h#newc...
src/objects-inl.h:174: bool Object::IsTheHole(Isolate* isolate) const {
Likewise, could we drop the second pair?

https://codereview.chromium.org/2028983002/diff/160001/src/objects-inl.h#newc...
src/objects-inl.h:2296: if (IsSmi()) return;
This is debug code, IMHO it should not return but go into the FATAL case below
for all the "false" cases.

https://codereview.chromium.org/2028983002/diff/160001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2028983002/diff/160001/src/objects.h#newcode1566
src/objects.h:1566: INLINE(bool IsTheHole(Isolate* isolate) const);
The predicates are already declared on the {Object} class and should be
inherited here. Also the implementation of those two is the same. Why do we need
to declare and implement them twice? Could we drop these two?

[Camillo Bruni, 2016-06-06 12:27:49]

https://codereview.chromium.org/2028983002/diff/160001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/2028983002/diff/160001/src/compiler/pipeline....
src/compiler/pipeline.cc:537: Isolate* isolate = script->GetIsolate();
On 2016/06/06 at 08:55:08, Michael Starzinger wrote:
> nit: info->isolate()

changed.

https://codereview.chromium.org/2028983002/diff/160001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/2028983002/diff/160001/src/factory.cc#newcode...
src/factory.cc:1305: if (prototype->IsTheHole(code->GetIsolate())) {
On 2016/06/06 at 08:55:08, Michael Starzinger wrote:
> nit: isolate()

done

https://codereview.chromium.org/2028983002/diff/160001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/2028983002/diff/160001/src/frames.cc#newcode1351
src/frames.cc:1351: Isolate* isolate = deopt_data->GetIsolate();
On 2016/06/06 at 08:55:08, Michael Starzinger wrote:
> nit: isolate()

done

https://codereview.chromium.org/2028983002/diff/160001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2028983002/diff/160001/src/objects-inl.h#newc...
src/objects-inl.h:2296: if (IsSmi()) return;
On 2016/06/06 at 08:55:08, Michael Starzinger wrote:
> This is debug code, IMHO it should not return but go into the FATAL case below
for all the "false" cases.

If it's a SMI it's a valid api call result => don't fail => early return.
So this should be fine.

[Camillo Bruni, 2016-06-06 12:28:34]

Leaving the duplicated methods on HeapObject in place for now.
Followup CLs will do further clean up.

[Camillo Bruni, 2016-06-06 12:28:41]

The CQ bit was checked by cbruni@chromium.org

[Camillo Bruni, 2016-06-06 12:28:42]

The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org,
ahaas@chromium.org, mstarzinger@chromium.org, verwaest@chromium.org,
yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2028983002/#ps220001
(title: "rebase master")

[commit-bot: I haz the power, 2016-06-06 12:28:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2028983002/220001

[Michael Starzinger, 2016-06-06 12:29:26]

https://codereview.chromium.org/2028983002/diff/160001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2028983002/diff/160001/src/objects-inl.h#newc...
src/objects-inl.h:2296: if (IsSmi()) return;
On 2016/06/06 12:27:49, Camillo Bruni wrote:
> On 2016/06/06 at 08:55:08, Michael Starzinger wrote:
> > This is debug code, IMHO it should not return but go into the FATAL case
below
> for all the "false" cases.
> 
> If it's a SMI it's a valid api call result => don't fail => early return.
> So this should be fine.

Acknowledged. Ah, oops, I misread the original code, sorry for the noise.

[commit-bot: I haz the power, 2016-06-06 12:58:20]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-06-06 13:00:55]

Description was changed from

==========
Introduce IsUndefined(Isolate*) and IsTheHole(Isolate*)

Passing in the isolate and pointer compare the instnance against the
corresponding constant is always faster than decoding the instance types.

BUG=
==========

to

==========
Introduce IsUndefined(Isolate*) and IsTheHole(Isolate*)

Passing in the isolate and pointer compare the instnance against the
corresponding constant is always faster than decoding the instance types.

BUG=

Committed: https://crrev.com/bc0798ca1a7e28eb82f03f40184b3b252f389fc4
Cr-Commit-Position: refs/heads/master@{#36744}
==========

[commit-bot: I haz the power, 2016-06-06 13:00:57]

Patchset 12 (id:??) landed as
https://crrev.com/bc0798ca1a7e28eb82f03f40184b3b252f389fc4
Cr-Commit-Position: refs/heads/master@{#36744}