Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(107)

Issue 14425011: api: Object::CachedProperty

Created:
6 years, 3 months ago by indutny
Modified:
6 years, 3 months ago
Reviewers:
danno, Toon Verwaest
CC:
v8-dev
Base URL:
gh:v8/v8.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : No APIs for Has #

Patch Set 3 : use handles #

Total comments: 1

Patch Set 4 : globalize reference #

Unified diffs Side-by-side diffs Delta from patch set Stats (+290 lines, -14 lines) Patch
M include/v8.h View 3 chunks +18 lines, -0 lines 0 comments Download
M src/api.cc View 3 chunks +50 lines, -0 lines 0 comments Download
M src/handles.h View 3 chunks +7 lines, -2 lines 0 comments Download
M src/handles.cc View 2 chunks +7 lines, -4 lines 0 comments Download
M src/objects.h View 1 2 2 chunks +29 lines, -0 lines 0 comments Download
M src/objects.cc View 1 2 3 1 chunk +131 lines, -0 lines 0 comments Download
M src/property.h View 2 chunks +10 lines, -1 line 0 comments Download
M src/runtime.h View 3 chunks +7 lines, -2 lines 0 comments Download
M src/runtime.cc View 1 2 4 chunks +13 lines, -5 lines 0 comments Download
M test/cctest/test-api.cc View 1 1 chunk +18 lines, -0 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
indutny
Note that it is a Work-In-Progress, and lacks implementation of `Has()` method so far.
6 years, 3 months ago (2013-04-28 21:08:28 UTC) #1
indutny
On 2013/04/28 21:08:28, indutny wrote: > Note that it is a Work-In-Progress, and lacks implementation ...
6 years, 3 months ago (2013-04-28 21:15:11 UTC) #2
indutny
On 2013/04/28 21:15:11, indutny wrote: > On 2013/04/28 21:08:28, indutny wrote: > > Note that ...
6 years, 3 months ago (2013-04-28 21:51:33 UTC) #3
indutny
On 2013/04/28 21:51:33, indutny wrote: > On 2013/04/28 21:15:11, indutny wrote: > > On 2013/04/28 ...
6 years, 3 months ago (2013-04-28 22:30:57 UTC) #4
indutny
On 2013/04/28 22:30:57, indutny wrote: > On 2013/04/28 21:51:33, indutny wrote: > > On 2013/04/28 ...
6 years, 3 months ago (2013-04-28 23:09:29 UTC) #5
danno
Fedor, I have some concerns about modifying the API like this (in addition to the ...
6 years, 3 months ago (2013-04-29 11:30:11 UTC) #6
indutny
6 years, 3 months ago (2013-04-29 11:34:50 UTC) #7
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.

Powered by Google App Engine
This is Rietveld 408576698