[Isolates] Use ::GetHeap() more often....

[Isolates] Use ::GetHeap() more often.

++ indicates v8 tests (higher is better, reference/runtime)
-- indicates closure tests (lower is better, usec)

This patch (ia32):
++Crypto: 1877.750000
--closure-json-raw: 357.500000
++EarleyBoyer: 5594.500000
++ws-longfact: 99.900000
--closure-datetime-raw: 187.250000
++Score (version 6 (candidate)): 530.000000
--closure-uri-raw: 174.000000
++Solve: 73.775000
++Splay: 329.000000
++RegExp: 1442.250000
++Richards: 2429.250000
--closure-queue2-raw: 3939.500000
--closure-eval2-raw: 119.750000
++ws-IEEE754Conv: 79.975000
++Genetic: 80.900000
++RayTrace: 4155.250000
--closure-log-formatter-raw: 234.250000
++ws-huffman: 91.525000
++DeltaBlue: 2242.500000

Isolates head r5232 (ia32):
++Crypto: 1853.750000
--closure-json-raw: 371.750000
++EarleyBoyer: 5499.250000
++ws-longfact: 96.575000
--closure-datetime-raw: 187.750000
++Score (version 6 (candidate)): 524.000000
--closure-uri-raw: 173.500000
++Solve: 79.925000
++Splay: 323.750000
++RegExp: 1408.250000
++Richards: 2451.250000
--closure-queue2-raw: 4028.750000
--closure-eval2-raw: 115.500000
++ws-IEEE754Conv: 74.225000
++Genetic: 79.000000
++RayTrace: 4040.000000
--closure-log-formatter-raw: 241.000000
++ws-huffman: 91.350000
++DeltaBlue: 2245.250000

This patch (x64):
++Crypto: 2001.750000
--closure-json-raw: 355.250000
++EarleyBoyer: 5079.500000
++ws-longfact: 101.500000
--closure-datetime-raw: 209.250000
++Score (version 6 (candidate)): 519.500000
--closure-uri-raw: 180.750000
++Solve: 85.325000
++Splay: 297.500000
++RegExp: 1386.500000
++Richards: 2281.500000
--closure-queue2-raw: 3591.500000
--closure-eval2-raw: 133.250000
++ws-IEEE754Conv: 90.275000
++Genetic: 87.750000
++RayTrace: 3687.500000
--closure-log-formatter-raw: 260.000000
++ws-huffman: 86.300000
++DeltaBlue: 1852.500000

Isolates head r5232 (x64):
++Crypto: 1990.000000
--closure-json-raw: 365.500000
++EarleyBoyer: 5091.000000
++ws-longfact: 104.000000
--closure-datetime-raw: 213.500000
++Score (version 6 (candidate)): 516.500000
--closure-uri-raw: 189.500000
++Solve: 82.000000
++Splay: 301.000000
++RegExp: 1381.750000
++Richards: 2263.000000
--closure-queue2-raw: 3723.500000
--closure-eval2-raw: 134.500000
++ws-IEEE754Conv: 86.000000
++Genetic: 89.500000
++RayTrace: 3633.750000
--closure-log-formatter-raw: 265.000000
++ws-huffman: 87.050000
++DeltaBlue: 1840.500000


Committed: http://code.google.com/p/v8/source/detail?r=5250

[zarko, 2010-08-10 23:08:31]

Thanks
  Luke

[Dmitry Titov, 2010-08-10 23:48:43]

Cool. Couple comments:

http://codereview.chromium.org/3139005/diff/1/2
File src/contexts.cc (right):

http://codereview.chromium.org/3139005/diff/1/2#newcode70
src/contexts.cc:70: Context* Context::global_context(Heap* heap) {
I wonder how much speedup this particular function brings. It seems the most
often case is even before GetHeap() would be called. Maybe nice to remove a
duplicate if speed is not gained.

http://codereview.chromium.org/3139005/diff/1/4
File src/objects-inl.h (right):

http://codereview.chromium.org/3139005/diff/1/4#newcode475
src/objects-inl.h:475: bool Object::IsGlobalContext(Heap* heap) {
Same Q about this duplicate: if replaced with IsGlobalContext(), does it affect
the perf? In some callsites perhaps this will allow to postpone getting the heap
(and leave compiler a bit more space for optimization)

http://codereview.chromium.org/3139005/diff/1/4#newcode1067
src/objects-inl.h:1067: #ifdef DEBUG
do we need this debug code? I believe ASSERT is already in Map::heap().

http://codereview.chromium.org/3139005/diff/1/4#newcode1077
src/objects-inl.h:1077: Heap* Object::SlowGetHeap() {
I measured similar function recently... It seems having that conditional
statement makes it pretty much same speed as simple Isolate::Current()->heap().
Perhaps can be removed?

http://codereview.chromium.org/3139005/diff/1/4#newcode1088
src/objects-inl.h:1088: Heap* Map::GetHeap() {
If we move ASSERT for GC into Map::heap(), that one can be used instead (I
assume you want to avoid extra dereference). Having non-virtual methods with the
same signature on derived objects seems a bit dangerous...

http://codereview.chromium.org/3139005/diff/1/4#newcode1095
src/objects-inl.h:1095: Heap* Map::MapGetHeap() {
this seems not used.

[zarko, 2010-08-11 18:25:25]

Thanks for the feedback! Here are the numbers for this version:

ia32:

++Crypto: 1880.750000
--closure-json-raw: 350.000000
++EarleyBoyer: 5645.750000
++ws-longfact: 103.500000
--closure-datetime-raw: 183.250000
++Score (version 6 (candidate)): 537.500000
--closure-uri-raw: 170.750000
++Solve: 77.075000
++Splay: 326.250000
++RegExp: 1462.500000
++Richards: 2450.250000
--closure-queue2-raw: 3815.500000
--closure-eval2-raw: 117.000000
++ws-IEEE754Conv: 85.050000
++Genetic: 81.350000
++RayTrace: 4099.000000
--closure-log-formatter-raw: 222.250000
++ws-huffman: 92.375000
++DeltaBlue: 2251.250000

x64:

++Crypto: 2041.500000
--closure-json-raw: 349.000000
++EarleyBoyer: 5174.000000
++ws-longfact: 104.925000
--closure-datetime-raw: 206.750000
++Score (version 6 (candidate)): 530.000000
--closure-uri-raw: 177.750000
++Solve: 88.025000
++Splay: 306.250000
++RegExp: 1413.500000
++Richards: 2306.750000
--closure-queue2-raw: 3551.500000
--closure-eval2-raw: 135.750000
++ws-IEEE754Conv: 92.200000
++Genetic: 88.125000
++RayTrace: 3751.500000
--closure-log-formatter-raw: 255.000000
++ws-huffman: 88.425000
++DeltaBlue: 1876.500000

http://codereview.chromium.org/3139005/diff/1/4
File src/objects-inl.h (right):

http://codereview.chromium.org/3139005/diff/1/4#newcode1088
src/objects-inl.h:1088: Heap* Map::GetHeap() {
On 2010/08/10 23:48:43, Dmitry Titov wrote:
> If we move ASSERT for GC into Map::heap(), that one can be used instead (I
> assume you want to avoid extra dereference). Having non-virtual methods with
the
> same signature on derived objects seems a bit dangerous...

I'm not sure what you mean by dangerous.

[Dmitry Titov, 2010-08-11 19:22:18]

> I'm not sure what you mean by dangerous.

It's just in general a good practice to only have the same-signature methods to
be either virtual or static on derived classes. A lot of hard-to-see bugs were
introduced by something like this:

class A {
  void Method() { printf("A"); }
}

class B : public A {
  void Method() { printf("B"); }
}


B* objectB = new B();
A* base = objectB;
base->Method();

This can be quite confusing, especially in a variant where someone decides to
make Method a 'virtual' - but only on B.

One example of the same rule is a requirement to have a virtual destructor on a
base class of derived hierarchy.

[Vitaly Repeshko, 2010-08-11 19:49:51]

LGTM

http://codereview.chromium.org/3139005/diff/8001/9002
File src/objects-inl.h (right):

http://codereview.chromium.org/3139005/diff/8001/9002#newcode462
src/objects-inl.h:462: return Object::IsHeapObject()
nit: "&&" should stay on this line.

http://codereview.chromium.org/3139005/diff/8001/9002#newcode470
src/objects-inl.h:470: && HeapObject::cast(this)->map() ==
Ditto.

http://codereview.chromium.org/3139005/diff/8001/9002#newcode554
src/objects-inl.h:554: && HeapObject::cast(this)->map() ==
Ditto.

http://codereview.chromium.org/3139005/diff/8001/9003
File src/objects.cc (right):

http://codereview.chromium.org/3139005/diff/8001/9003#newcode99
src/objects.cc:99: if (IsTrue()) return
HeapObject::cast(this)->GetHeap()->true_value();
These two first lines are a bit strange now. If Is{True,False} accepted some
broad class of objects (not just {true,false}_value) e.g. 42 is true and 0 is
false, then HeapObject::cast would be dangerous. But given that they only accept
{true,false}_value we can return "this" instead.

http://codereview.chromium.org/3139005/diff/8001/9003#newcode2885
src/objects.cc:2885: || HEAP->isolate()->MayNamedAccess(this, name,
v8::ACCESS_SET));
I don't think it's any better than Isolate::Current().

http://codereview.chromium.org/3139005/diff/8001/9003#newcode8291
src/objects.cc:8291: Heap* heap = Isolate::Current()->heap();
Dictionary is a FixedArray. GetHeap() should work here.

http://codereview.chromium.org/3139005/diff/8001/9003#newcode8384
src/objects.cc:8384: Heap* heap = Isolate::Current()->heap();
Ditto.

http://codereview.chromium.org/3139005/diff/8001/9003#newcode8594
src/objects.cc:8594: Heap* heap = Isolate::Current()->heap();
Ditto.

[zarko, 2010-08-11 23:53:28]

Thanks again! I removed Map::GetHeap(). (It didn't help perf above what appeared
to be noise, so I figured that it wasn't worth the potential confusion.)