Get rid of unnecessary handle management when invoking interceptors.

Get rid of unnecessary handle management when invoking interceptors.


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

[Kasper Lund, 2009-07-17 08:27:20]

If you clean this up a bit (too long lines) and remove
GetInterceptorPropertyWithLookupHint from objects.{h,cc} then this LGTM. Of
course you also have to make the payload an old space object.

http://codereview.chromium.org/155682/diff/4/1005
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/155682/diff/4/1005#newcode515
Line 515: v8::NamedPropertyGetter getter =
v8::ToCData<v8::NamedPropertyGetter>(interceptor->getter());
Line too long.

http://codereview.chromium.org/155682/diff/4/1006
File src/stub-cache.cc (left):

http://codereview.chromium.org/155682/diff/4/1006#oldcode799
Line 799: Object* result = holder->GetInterceptorPropertyWithLookupHint(
Is GetInterceptorPropertyWithLookupHint only used from here? If it isn't used
anymore it should be removed.

http://codereview.chromium.org/155682/diff/4/1006
File src/stub-cache.cc (right):

http://codereview.chromium.org/155682/diff/4/1006#newcode797
Line 797: v8::NamedPropertyGetter getter =
reinterpret_cast<v8::NamedPropertyGetter>(Smi::cast(args[5])->value() << 1);
Lint too long.

http://codereview.chromium.org/155682/diff/4/1006#newcode803
Line 803: LOG(ApiNamedPropertyAccess("interceptor-named-get", *holder_handle,
*name_handle));
Line too long?

[Kasper Lund, 2009-07-17 08:46:27]

Retracting my LGTM:

http://codereview.chromium.org/155682/diff/4/1005
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/155682/diff/4/1005#newcode516
Line 516: __ push(Immediate(reinterpret_cast<int>(getter)));
This is not GC safe if the getter isn't a smi. This needs more work.

[antonm, 2009-07-28 12:37:17]

Kasper,

I hope that addresses all your comments.

All,

I dropped couple of micro-optimizations (direct store of data if in old space,
storing of getter) to make callbacks and interceptors look alike.  Probably
would add into both with a separate CL if it buys us even a slightest bit.

And I'd be glad to see it in 1.2

http://codereview.chromium.org/155682/diff/4/1005
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/155682/diff/4/1005#newcode515
Line 515: v8::NamedPropertyGetter getter =
v8::ToCData<v8::NamedPropertyGetter>(interceptor->getter());
On 2009/07/17 08:27:20, Kasper Lund wrote:
> Line too long.

Done.

http://codereview.chromium.org/155682/diff/4/1005#newcode516
Line 516: __ push(Immediate(reinterpret_cast<int>(getter)));
On 2009/07/17 08:46:27, Kasper Lund wrote:
> This is not GC safe if the getter isn't a smi. This needs more work.

Thanks for spotting this.  What about that variant?

[Kasper Lund, 2009-07-28 12:52:24]

I think my main concerns are addressed. Christian or Vitaly should take a final
look too.

[Christian Plesner Hansen, 2009-07-28 13:44:56]

Lgtm

[Vitaly Repeshko, 2009-07-28 13:54:24]

LGTM.

http://codereview.chromium.org/155682/diff/2027/2031
File src/objects.cc (right):

http://codereview.chromium.org/155682/diff/2027/2031#newcode6038
Line 6038: HandleScope scope;
Consider moving handle stuff one level up (as it was before). The move doesn't
slow down the case we care about and makes the code considerably simpler.

http://codereview.chromium.org/155682/diff/2027/2031#newcode6067
Line 6067: Object* result = holder->GetPropertyPostInterceptor(receiver, name,
attributes);
Line too long.

http://codereview.chromium.org/155682/diff/2027/2029
File src/stub-cache.cc (right):

http://codereview.chromium.org/155682/diff/2027/2029#newcode807
Line 807: {
Document the block, e.g. "Use the interceptor getter".

[antonm, 2009-07-28 14:22:01]

Thank you very much for review!

Submitting.

http://codereview.chromium.org/155682/diff/2027/2031
File src/objects.cc (right):

http://codereview.chromium.org/155682/diff/2027/2031#newcode6038
Line 6038: HandleScope scope;
On 2009/07/28 13:54:25, Vitaly wrote:
> Consider moving handle stuff one level up (as it was before). The move doesn't
> slow down the case we care about and makes the code considerably simpler.

Done.

http://codereview.chromium.org/155682/diff/2027/2031#newcode6067
Line 6067: Object* result = holder->GetPropertyPostInterceptor(receiver, name,
attributes);
On 2009/07/28 13:54:25, Vitaly wrote:
> Line too long.

Done.

http://codereview.chromium.org/155682/diff/2027/2029
File src/stub-cache.cc (right):

http://codereview.chromium.org/155682/diff/2027/2029#newcode807
Line 807: {
On 2009/07/28 13:54:25, Vitaly wrote:
> Document the block, e.g. "Use the interceptor getter".

Done.