api: Object::CachedProperty

api: Object::CachedProperty

This is a work in progress, just want to share it with you guys to get some
critics and comments about it!

From my benchmarks property lookup from C++ seems to be faster by 10-23% with
this patch.

BUG=
R=danno@chromium.org

[indutny, 2013-04-28 21:08:28]

Note that it is a Work-In-Progress, and lacks implementation of `Has()` method
so far.

[indutny, 2013-04-28 21:15:11]

On 2013/04/28 21:08:28, indutny wrote:
> Note that it is a Work-In-Progress, and lacks implementation of `Has()` method
> so far.

Ok, I decided to avoid implementing `Has()`.

[indutny, 2013-04-28 21:51:33]

On 2013/04/28 21:15:11, indutny wrote:
> On 2013/04/28 21:08:28, indutny wrote:
> > Note that it is a Work-In-Progress, and lacks implementation of `Has()`
method
> > so far.
> 
> Ok, I decided to avoid implementing `Has()`.

It seems to be ready for review, I'll do benchmarks with node.js soon.

[indutny, 2013-04-28 22:30:57]

On 2013/04/28 21:51:33, indutny wrote:
> On 2013/04/28 21:15:11, indutny wrote:
> > On 2013/04/28 21:08:28, indutny wrote:
> > > Note that it is a Work-In-Progress, and lacks implementation of `Has()`
> method
> > > so far.
> > 
> > Ok, I decided to avoid implementing `Has()`.
> 
> It seems to be ready for review, I'll do benchmarks with node.js soon.

It fails sometimes:

#3  0x0000000100733c99 in v8::internal::OS::Abort () at
../deps/v8/src/platform-macos.cc:182
#4  0x00000001002c9db4 in V8_Fatal (file=0x10080f4a5
"../deps/v8/src/transitions-inl.h", line=170, format=0x1007e1554 "CHECK(%s)
failed") at ../deps/v8/src/checks.cc:59
#5  0x00000001004fc504 in v8::internal::TransitionArray::GetTarget
(this=0x3e8e12c3c729, transition_number=0) at transitions-inl.h:170
#6  0x0000000100514ef7 in v8::internal::LookupResult::GetTransitionTarget
(this=0x7fff5fbf6420, map=0x1d3a30e12c69) at property.h:377
#7  0x00000001004f685d in v8::internal::LookupResult::GetTransitionTarget
(this=0x7fff5fbf6420) at property.h:381
#8  0x00000001004c21a0 in v8::internal::JSObject::SetPropertyForResult
(this=0x17e58e693521, lookup=0x7fff5fbf6420, name_raw=0x514ad31c801,
value_raw=0x1b11b8f057a9, attributes=NONE,
strict_mode=v8::internal::kNonStrictMode,
store_mode=v8::internal::JSReceiver::MAY_BE_STORE_FROM_KEYED) at
../deps/v8/src/objects.cc:3204
#9  0x00000001004bcd52 in v8::internal::JSReceiver::SetProperty
(this=0x17e58e693521, result=0x7fff5fbf6420, key=0x514ad31c801,
value=0x1b11b8f057a9, attributes=NONE, strict_mode=v8::internal::kNonStrictMode,
store_mode=v8::internal::JSReceiver::MAY_BE_STORE_FROM_KEYED) at
../deps/v8/src/objects.cc:2767
#10 0x00000001004e9b90 in v8::internal::LookupCache::SetProperty
(this=0x101410e10, obj={location_ = 0x102020be0}, name={location_ =
0x102020680}, value={location_ = 0x7fff5fbf6b90}) at
../deps/v8/src/objects.cc:14615
#11 0x0000000100574a8a in v8::internal::Runtime::SetObjectProperty
(isolate=0x101809400, object={location_ = 0x102020be0}, key={location_ =
0x102020680}, value={location_ = 0x7fff5fbf6b90}, attr=NONE,
strict_mode=v8::internal::kNonStrictMode, cache=0x101410e10) at
../deps/v8/src/runtime.cc:4467
#12 0x000000010036262d in v8::internal::SetProperty (isolate=0x101809400,
object={location_ = 0x102020be0}, key={location_ = 0x102020680},
value={location_ = 0x7fff5fbf6b90}, attributes=NONE,
strict_mode=v8::internal::kNonStrictMode, cache=0x101410e10) at
../deps/v8/src/handles.cc:235
#13 0x0000000100268d72 in v8::Object::Set (this=0x102020be0, key={cache_ =
0x101410e10, key_ = {<v8::Handle<v8::String>> = {val_ = 0x102020680}, <No data
fields>}}, value={val_ = 0x7fff5fbf6b90}, attribs=v8::None) at
../deps/v8/src/api.cc:3079
#14 0x000000010000bee4 in node::HandleWrap::Close (args=@0x7fff5fbf69c8) at
../src/handle_wrap.cc:101
#15 0x00000001002c4c0d in HandleApiCallHelper<false>
(args={<v8::internal::Arguments> = {<v8::internal::Embedded> = {<No data
fields>}, length_ = 3, arguments_ = 0x7fff5fbf6b98}, <No data fields>},
isolate=0x101809400) at ../deps/v8/src/builtins.cc:1327
#16 0x00000001002c4813 in Builtin_Impl_HandleApiCall
(args={<v8::internal::Arguments> = {<v8::internal::Embedded> = {<No data
fields>}, length_ = 3, arguments_ = 0x7fff5fbf6b98}, <No data fields>},
isolate=0x101809400) at ../deps/v8/src/builtins.cc:1345
#17 0x00000001002be0b7 in Builtin_HandleApiCall (args={<v8::internal::Arguments>
= {<v8::internal::Embedded> = {<No data fields>}, length_ = 3, arguments_ =
0x7fff5fbf6b98}, <No data fields>}, isolate=0x101809400) at
../deps/v8/src/builtins.cc:1344

P.S. Ignore line numbers, they're different because this patch was applied not
to v8's bleeding_edge.

[indutny, 2013-04-28 23:09:29]

On 2013/04/28 22:30:57, indutny wrote:
> On 2013/04/28 21:51:33, indutny wrote:
> > On 2013/04/28 21:15:11, indutny wrote:
> > > On 2013/04/28 21:08:28, indutny wrote:
> > > > Note that it is a Work-In-Progress, and lacks implementation of `Has()`
> > method
> > > > so far.
> > > 
> > > Ok, I decided to avoid implementing `Has()`.
> > 
> > It seems to be ready for review, I'll do benchmarks with node.js soon.
> 
> It fails sometimes:
> 
> #3  0x0000000100733c99 in v8::internal::OS::Abort () at
> ../deps/v8/src/platform-macos.cc:182
> #4  0x00000001002c9db4 in V8_Fatal (file=0x10080f4a5
> "../deps/v8/src/transitions-inl.h", line=170, format=0x1007e1554 "CHECK(%s)
> failed") at ../deps/v8/src/checks.cc:59
> #5  0x00000001004fc504 in v8::internal::TransitionArray::GetTarget
> (this=0x3e8e12c3c729, transition_number=0) at transitions-inl.h:170
> #6  0x0000000100514ef7 in v8::internal::LookupResult::GetTransitionTarget
> (this=0x7fff5fbf6420, map=0x1d3a30e12c69) at property.h:377
> #7  0x00000001004f685d in v8::internal::LookupResult::GetTransitionTarget
> (this=0x7fff5fbf6420) at property.h:381
> #8  0x00000001004c21a0 in v8::internal::JSObject::SetPropertyForResult
> (this=0x17e58e693521, lookup=0x7fff5fbf6420, name_raw=0x514ad31c801,
> value_raw=0x1b11b8f057a9, attributes=NONE,
> strict_mode=v8::internal::kNonStrictMode,
> store_mode=v8::internal::JSReceiver::MAY_BE_STORE_FROM_KEYED) at
> ../deps/v8/src/objects.cc:3204
> #9  0x00000001004bcd52 in v8::internal::JSReceiver::SetProperty
> (this=0x17e58e693521, result=0x7fff5fbf6420, key=0x514ad31c801,
> value=0x1b11b8f057a9, attributes=NONE,
strict_mode=v8::internal::kNonStrictMode,
> store_mode=v8::internal::JSReceiver::MAY_BE_STORE_FROM_KEYED) at
> ../deps/v8/src/objects.cc:2767
> #10 0x00000001004e9b90 in v8::internal::LookupCache::SetProperty
> (this=0x101410e10, obj={location_ = 0x102020be0}, name={location_ =
> 0x102020680}, value={location_ = 0x7fff5fbf6b90}) at
> ../deps/v8/src/objects.cc:14615
> #11 0x0000000100574a8a in v8::internal::Runtime::SetObjectProperty
> (isolate=0x101809400, object={location_ = 0x102020be0}, key={location_ =
> 0x102020680}, value={location_ = 0x7fff5fbf6b90}, attr=NONE,
> strict_mode=v8::internal::kNonStrictMode, cache=0x101410e10) at
> ../deps/v8/src/runtime.cc:4467
> #12 0x000000010036262d in v8::internal::SetProperty (isolate=0x101809400,
> object={location_ = 0x102020be0}, key={location_ = 0x102020680},
> value={location_ = 0x7fff5fbf6b90}, attributes=NONE,
> strict_mode=v8::internal::kNonStrictMode, cache=0x101410e10) at
> ../deps/v8/src/handles.cc:235
> #13 0x0000000100268d72 in v8::Object::Set (this=0x102020be0, key={cache_ =
> 0x101410e10, key_ = {<v8::Handle<v8::String>> = {val_ = 0x102020680}, <No data
> fields>}}, value={val_ = 0x7fff5fbf6b90}, attribs=v8::None) at
> ../deps/v8/src/api.cc:3079
> #14 0x000000010000bee4 in node::HandleWrap::Close (args=@0x7fff5fbf69c8) at
> ../src/handle_wrap.cc:101
> #15 0x00000001002c4c0d in HandleApiCallHelper<false>
> (args={<v8::internal::Arguments> = {<v8::internal::Embedded> = {<No data
> fields>}, length_ = 3, arguments_ = 0x7fff5fbf6b98}, <No data fields>},
> isolate=0x101809400) at ../deps/v8/src/builtins.cc:1327
> #16 0x00000001002c4813 in Builtin_Impl_HandleApiCall
> (args={<v8::internal::Arguments> = {<v8::internal::Embedded> = {<No data
> fields>}, length_ = 3, arguments_ = 0x7fff5fbf6b98}, <No data fields>},
> isolate=0x101809400) at ../deps/v8/src/builtins.cc:1345
> #17 0x00000001002be0b7 in Builtin_HandleApiCall
(args={<v8::internal::Arguments>
> = {<v8::internal::Embedded> = {<No data fields>}, length_ = 3, arguments_ =
> 0x7fff5fbf6b98}, <No data fields>}, isolate=0x101809400) at
> ../deps/v8/src/builtins.cc:1344
> 
> P.S. Ignore line numbers, they're different because this patch was applied not
> to v8's bleeding_edge.

Seems like its caching TRANSITION lookup with index=0 and after cache hit
happens map has 0 transitions... how is it even possible?

[danno, 2013-04-29 11:30:11]

Fedor, I have some concerns about modifying the API like this (in addition to
the fundamental implementation issue below). It pollutes the existing Get/Set
APIs and adds a fair amount of duplication for an arguably non-core use case. If
performance is really an issue, then you probably want something specialized to
minimize the overhead even more.

After chatting with Slava, I think a better alternative would add API points to
wrap ICs and encapsulate fast property access. He suggested something like:

typedef Handle<Value> (*PropertyGetter)(Isolate* isolate, Handle<Object> obj);
PropertyGetter CreateGetter(Isolate* isolate, Handle<String> name);

https://codereview.chromium.org/14425011/diff/3002/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/14425011/diff/3002/src/objects.cc#newcode15078
src/objects.cc:15078: Map* map_;
Oh no! These are naked heap pointers. This approach fundamentally won't work,
you don't have any guarantees that these maps are alive after adding them to the
cache. V8 actively GCs maps and transitions that are dead to combat memory
leaks, and dangling Map* pointers _will_ bite you sooner or later (that's almost
certainly the source of your assert).

[indutny, 2013-04-29 11:34:50]

On 2013/04/29 11:30:11, danno wrote:
> Fedor, I have some concerns about modifying the API like this (in addition to
> the fundamental implementation issue below). It pollutes the existing Get/Set
> APIs and adds a fair amount of duplication for an arguably non-core use case.
If
> performance is really an issue, then you probably want something specialized
to
> minimize the overhead even more.
> 
> After chatting with Slava, I think a better alternative would add API points
to
> wrap ICs and encapsulate fast property access. He suggested something like:
> 
> typedef Handle<Value> (*PropertyGetter)(Isolate* isolate, Handle<Object> obj);
> PropertyGetter CreateGetter(Isolate* isolate, Handle<String> name);
> 
> https://codereview.chromium.org/14425011/diff/3002/src/objects.cc
> File src/objects.cc (right):
> 
> https://codereview.chromium.org/14425011/diff/3002/src/objects.cc#newcode15078
> src/objects.cc:15078: Map* map_;
> Oh no! These are naked heap pointers. This approach fundamentally won't work,
> you don't have any guarantees that these maps are alive after adding them to
the
> cache. V8 actively GCs maps and transitions that are dead to combat memory
> leaks, and dangling Map* pointers _will_ bite you sooner or later (that's
almost
> certainly the source of your assert).

Oh, I just forgot to upload recent changes, but still it doesn't fix problem.

Anyway, IC seems to be much better approach for me, though, I'm afraid of, lets
call it, PrepareBeforeEnter execution costs. I can probably help you with this,
but please advise me the best approach for it.