Stub Cache: speed up load callback accessor by allocating data handle on stack.

Stub Cache: speed up load callback accessor by allocating data handle on stack.

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

[antonm, 2009-07-23 21:12:57]

overall LGTM.

And very neat trick to solve the problem of data in new space.

Some minor comments.  You might want to compare it to
http://codereview.chromium.org/155682 which uses your cool trick as well

http://codereview.chromium.org/160041/diff/1001/1002
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/160041/diff/1001/1002#newcode473
Line 473: __ ldr(reg, FieldMemOperand(ip, AccessorInfo::kDataOffset));
why not push data immediately if data() belongs to new space?

http://codereview.chromium.org/160041/diff/1001/1003
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/160041/diff/1001/1003#newcode451
Line 451: __ mov(reg, Immediate(Handle<AccessorInfo>(callback)));  // callback
data
why not __ push(Immediate....)?

http://codereview.chromium.org/160041/diff/1001/1003#newcode453
Line 453: __ push(FieldOperand(reg, AccessorInfo::kDataOffset));
ditto (shortcutting if data is not in new space)

http://codereview.chromium.org/160041/diff/1001/1004
File src/stub-cache.cc (right):

http://codereview.chromium.org/160041/diff/1001/1004#newcode742
Line 742: Address getter_address = v8::ToCData<Address>(callback->getter());
maybe compile getter_address into stub as well?  As you told me on ia32 it
should be probably safe as the address would be aligned, do you know if it's
still the case for ARM?  And x64?

http://codereview.chromium.org/160041/diff/1001/1004#newcode754
Line 754: HandleScope scope;
maybe move scope just before entering fun?  for example, to reduce its scope
(sorry) and make it easier for maintainer to understand that scope is only
needed for fun?

[iposva, 2009-07-24 00:30:07]

http://codereview.chromium.org/160041/diff/1001/1002
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/160041/diff/1001/1002#newcode473
Line 473: __ ldr(reg, FieldMemOperand(ip, AccessorInfo::kDataOffset));
On 2009/07/23 21:12:58, antonm wrote:
> why not push data immediately if data() belongs to new space?

Maybe because on ARM loading an Object constant is also a load instruction from
the constant pool? No difference and you save on code/constant pool space.

http://codereview.chromium.org/160041/diff/1001/1004
File src/stub-cache.cc (right):

http://codereview.chromium.org/160041/diff/1001/1004#newcode750
Line 750: LOG(ApiNamedPropertyAccess("load", *recv, *name));
Do you really intend to log for every single property access with a callback? I
know that code was there before, but maybe it is time to evaluate if we ever are
looking at this log output?

http://codereview.chromium.org/160041/diff/1001/1004#newcode754
Line 754: HandleScope scope;
On 2009/07/23 21:12:58, antonm wrote:
> maybe move scope just before entering fun?  for example, to reduce its scope
> (sorry) and make it easier for maintainer to understand that scope is only
> needed for fun?

And where would the result handle on the next line be allocated?

[antonm, 2009-07-24 02:43:21]

http://codereview.chromium.org/160041/diff/1001/1002
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/160041/diff/1001/1002#newcode473
Line 473: __ ldr(reg, FieldMemOperand(ip, AccessorInfo::kDataOffset));
On 2009/07/24 00:30:07, iposva wrote:
> On 2009/07/23 21:12:58, antonm wrote:
> > why not push data immediately if data() belongs to new space?
> 
> Maybe because on ARM loading an Object constant is also a load instruction
from
> the constant pool? No difference and you save on code/constant pool space.
> 

Sorry, didn't know that and was under induction of compiling getter_adder into
the stub.  In this case you don't load callback and thus loading data stores you
one operation.

http://codereview.chromium.org/160041/diff/1001/1004
File src/stub-cache.cc (right):

http://codereview.chromium.org/160041/diff/1001/1004#newcode754
Line 754: HandleScope scope;
On 2009/07/24 00:30:07, iposva wrote:
> On 2009/07/23 21:12:58, antonm wrote:
> > maybe move scope just before entering fun?  for example, to reduce its scope
> > (sorry) and make it easier for maintainer to understand that scope is only
> > needed for fun?
> 
> And where would the result handle on the next line be allocated?

Correct me if I'm wrong, but v8::Handle::Handle() doesn't require scope.

[Christian Plesner Hansen, 2009-07-24 07:33:25]

Lgtm

http://codereview.chromium.org/160041/diff/1001/1004
File src/stub-cache.cc (right):

http://codereview.chromium.org/160041/diff/1001/1004#newcode741
Line 741: Handle<Object> data = args.at<Object>(3);
A further optimization could be to change the implementation of AccessorInfo to
hold a pointer directly into the stack and then extract the values on access
rather than up front.  That would also make things easier if we want to call the
function directly from generated code.

I'm not suggesting doing it as part of this CL but that's something we might
consider later.

http://codereview.chromium.org/160041/diff/1001/1004#newcode750
Line 750: LOG(ApiNamedPropertyAccess("load", *recv, *name));
I agree.  It looks like we're already losing performance because of api logging
and I don't believe anyone is using it.

http://codereview.chromium.org/160041/diff/1001/1004#newcode754
Line 754: HandleScope scope;
If you move the handle scope into the inner scope the value assigned to 'result'
will become invalid as soon as you leave the inner scope.

[antonm, 2009-07-24 08:59:34]

http://codereview.chromium.org/160041/diff/1001/1004
File src/stub-cache.cc (right):

http://codereview.chromium.org/160041/diff/1001/1004#newcode754
Line 754: HandleScope scope;
On 2009/07/24 07:33:25, Christian Plesner Hansen wrote:
> If you move the handle scope into the inner scope the value assigned to
'result'
> will become invalid as soon as you leave the inner scope.

Aha, stupid me.

[Vitaly Repeshko, 2009-07-24 13:45:38]

Thanks!

-- Vitaly

http://codereview.chromium.org/160041/diff/1001/1002
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/160041/diff/1001/1002#newcode473
Line 473: __ ldr(reg, FieldMemOperand(ip, AccessorInfo::kDataOffset));
On 2009/07/24 02:43:21, antonm wrote:
> On 2009/07/24 00:30:07, iposva wrote:
> > On 2009/07/23 21:12:58, antonm wrote:
> > > why not push data immediately if data() belongs to new space?
> > 
> > Maybe because on ARM loading an Object constant is also a load instruction
> from
> > the constant pool? No difference and you save on code/constant pool space.
> > 
> 
> Sorry, didn't know that and was under induction of compiling getter_adder into
> the stub.  In this case you don't load callback and thus loading data stores
you
> one operation.

I didn't measure performance here, but based on what Ivan said and my
measurements for x86 I decided to leave simpler code here as well.

http://codereview.chromium.org/160041/diff/1001/1003
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/160041/diff/1001/1003#newcode451
Line 451: __ mov(reg, Immediate(Handle<AccessorInfo>(callback)));  // callback
data
On 2009/07/23 21:12:58, antonm wrote:
> why not __ push(Immediate....)?

I need this data in a register anyway (to support the push below). Current
approach saves us a relocation entry.

http://codereview.chromium.org/160041/diff/1001/1003#newcode453
Line 453: __ push(FieldOperand(reg, AccessorInfo::kDataOffset));
On 2009/07/23 21:12:58, antonm wrote:
> ditto (shortcutting if data is not in new space)

I tried it and my measurements didn't show any difference in performance. So I'd
prefer to keep this simple.

http://codereview.chromium.org/160041/diff/1001/1004
File src/stub-cache.cc (right):

http://codereview.chromium.org/160041/diff/1001/1004#newcode741
Line 741: Handle<Object> data = args.at<Object>(3);
On 2009/07/24 07:33:25, Christian Plesner Hansen wrote:
> A further optimization could be to change the implementation of AccessorInfo
to
> hold a pointer directly into the stack and then extract the values on access
> rather than up front.  That would also make things easier if we want to call
the
> function directly from generated code.
> 
> I'm not suggesting doing it as part of this CL but that's something we might
> consider later.

Sure. After this patch is in, I was going to experiment with what is described
in the NOTE comment below.

http://codereview.chromium.org/160041/diff/1001/1004#newcode742
Line 742: Address getter_address = v8::ToCData<Address>(callback->getter());
On 2009/07/23 21:12:58, antonm wrote:
> maybe compile getter_address into stub as well?  As you told me on ia32 it
> should be probably safe as the address would be aligned, do you know if it's
> still the case for ARM?  And x64?

I think this essentially means moving a few instructions from here to the stub
cache code. So I don't see how it makes a difference at this point.

http://codereview.chromium.org/160041/diff/1001/1004#newcode750
Line 750: LOG(ApiNamedPropertyAccess("load", *recv, *name));
On 2009/07/24 07:33:25, Christian Plesner Hansen wrote:
> I agree.  It looks like we're already losing performance because of api
logging
> and I don't believe anyone is using it.

Well, this does cost us a bit, but I'm not sure I should just remove logging.
What about defining something like LOG_VERBOSE macro (for performance-critical
places) that gets compiled to nothing without a special define? I could do this
for all similar places.

[Vitaly Repeshko, 2009-07-24 15:43:55]

http://codereview.chromium.org/160041/diff/1001/1004
File src/stub-cache.cc (right):

http://codereview.chromium.org/160041/diff/1001/1004#newcode742
Line 742: Address getter_address = v8::ToCData<Address>(callback->getter());
On 2009/07/24 13:45:38, Vitaly wrote:
> On 2009/07/23 21:12:58, antonm wrote:
> > maybe compile getter_address into stub as well?  As you told me on ia32 it
> > should be probably safe as the address would be aligned, do you know if it's
> > still the case for ARM?  And x64?
> 
> I think this essentially means moving a few instructions from here to the stub
> cache code. So I don't see how it makes a difference at this point.

Anton pointed out in offline discussion that we store the address indirectly
using a Proxy object. Unfortunately, putting it in the stub code directly
besides requiring extra care with respect to alignment doesn't make any
difference according to my measurements.

[Vitaly Repeshko, 2009-07-27 09:59:42]

This is now submitted. I'll continue working on LOG stuff and will experiment
with moving more code into generated stub.

Thanks,
Vitaly