Thanks a lot, everyone!
Submitting
http://codereview.chromium.org/140069/diff/8019/8028
File src/heap.h (right):
http://codereview.chromium.org/140069/diff/8019/8028#newcode113
Line 113: V(Object, none_value, NoneValue)
\
On 2009/07/29 10:02:37, Kasper Lund wrote:
> I think you
You should change the none_value name. It's too generic. Other than that
it still LGTM.
http://codereview.chromium.org/140069/diff/8019/8028
File src/heap.h (right):
http://codereview.chromium.org/140069/diff/8019/8028#newcode113
Line 113: V(Object, none_value, NoneValue)
\
I thi
Oh, yes, but it changed a bit---merged changes in load interceptors +
added none_value (as per chat with you).
yours,
anton.
On Wed, Jul 29, 2009 at 1:44 PM, Kasper Lund wrote:
> Didn't I already LGTM this one?
>
> On Wed, Jul 29, 2009 at 11:37 AM, Anton Muhin wrote:
>> Guys,
>>
>> I merged it w
Didn't I already LGTM this one?
On Wed, Jul 29, 2009 at 11:37 AM, Anton Muhin wrote:
> Guys,
>
> I merged it with the most recent trunk. I'm not planning it to be
> cherry picked into 1.2 (as was discussed with Kasper before).
>
> yours,
> anton.
>
> On Wed, Jul 29, 2009 at 1:35 PM, wrote:
>> h
Guys,
I merged it with the most recent trunk. I'm not planning it to be
cherry picked into 1.2 (as was discussed with Kasper before).
yours,
anton.
On Wed, Jul 29, 2009 at 1:35 PM, wrote:
> http://codereview.chromium.org/140069
>
--~--~-~--~~~---~--~~
v8-dev m
http://codereview.chromium.org/140069
--~--~-~--~~~---~--~~
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
-~--~~~~--~~--~--~---
LGTM.
http://codereview.chromium.org/140069
--~--~-~--~~~---~--~~
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
-~--~~~~--~~--~--~---
Kasper, tnx a lot for review!
Hopefully addressed everything.
I reworked a bit case with callbacks and invalidation as NULL setter
make property readonly and __proto__ games I used to play apparently
invalidate maps and thus don't allow to prove necessity of
CheckPrototypes.
When you're fine wi
If you change the CheckMaps to CheckPrototypes and add a few tests of
why that is necessary, it LGTM.
http://codereview.chromium.org/140069/diff/6055/6057
File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/140069/diff/6055/6057#newcode365
Line 365: if (lookup->type() == FI
Guys, sorry for bothering you w/ this CL another time, but in a hard
way I learned that number field of LookupResult cannot be cached (due
to transitions), so the thing I postponed for next CL had to be merged
into this one. May you have another look?
All v8 tests (except for regress-75 in debug
And Kasper,
I gather I should use CheckPrototypes when doing a second check, correct?
yours,
anton.
On Tue, Jul 14, 2009 at 10:46 PM, Anton Muhin wrote:
> Guys, sorry for bothering you w/ this CL another time, but in a hard
> way I learned that number field of LookupResult cannot be cached (due
You should port the real thing to ARM once you have landed this change
and everything looks fine.
Cheers, -- Mads
On Thu, Jul 9, 2009 at 8:06 AM, wrote:
> LGTM
>
> http://codereview.chromium.org/140069
>
--~--~-~--~~~---~--~~
v8-dev mailing list
v8-dev@googleg
LGTM
http://codereview.chromium.org/140069
--~--~-~--~~~---~--~~
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
-~--~~~~--~~--~--~---
Apparently it was a bad idea to push holder out of internal frame---with
stuff moved into the frame tests pass.
x64 and arm works fine.
May you have another look?
http://codereview.chromium.org/140069
--~--~-~--~~~---~--~~
v8-dev mailing list
v8-dev@googlegroups
Mads, thanks a lot for review!
I've reran tests and curiously release mode tests for ia32 failed. I
don't know precise reason for failure yet (only threaded test fails),
but that made me think if my approach is sane. For example consider the
case of CallIC. Imagine that call to get property wi
LGTM once you make it compile on ARM and x64.
http://codereview.chromium.org/140069/diff/5032/6015
File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/140069/diff/5032/6015#newcode437
Line 437: // Do tail-call to LoadPropertyByLookupResult
End comment with period.
http://c
Mads, tnx a lot for review!
I've added more tests to cover all the cases of cacheable lookup
(hopefully regular one is tested enough already).
Regarding IsContextual. Thanks a lot for explanations. Is there a way
to access this information from StubCompiler? In this case I'd able to
compile p
A couple of thinks I forgot:
I think I missed the place where you actually use CheckMaps with a NULL
holder?
We need tests that cover all cases in the generated code. I would
recommend putting in "__ int3();" in one branch at a time and make sure
that you have a test that hits that branch.
Tha
Another batch of comments. I think we are getting there. Thanks for
doing the statistics and simplifications based on them.
IsContextual really means that we are performing a global load. Global
loads of properties that are not there throws ReferenceErrors. Loads of
other properties that are
And forgotten: I tried to refactor some more common logic. Please let
me know if you like this new approach or prefer older one or looking
for more sharing.
yours,
anton.
On Fri, Jul 3, 2009 at 4:09 AM, wrote:
> Mads, I hope I've addressed all your comments.
>
> Tnx a lot for suggestion to sim
Mads, I hope I've addressed all your comments.
Tnx a lot for suggestion to simplify logic based on usage: I sent a doc
with some stats to you and Kasper.
I hope this CL might be simplified another bit: I think ic.is_contextual
(used to determine if we should throw an exception or return failed
v
http://codereview.chromium.org/140069/diff/3001/3002
File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/140069/diff/3001/3002#newcode429
Line 429: masm->CheckMaps(holder, eax, GetLastJSObjectProto(holder),
On 2009/06/30 09:22:28, Mads Ager wrote:
> Can we modify CheckMaps t
Overall this looks good. There is a lot of code for generating the IC
stubs now and as you write, we need to make sure that it is all tested.
I wonder if more of it can be factored out?
Also, I would like to have some counters added so we can get information
about how often the different parts o
Guys, may you have a look at another pass?
I think that no getter interceptor case should be lifted higher in the
stack (ic.cc) and this CL lacks tests and cleanups, but I'd like to get
your LGTM on stub part first.
tia and yours,
anton.
http://codereview.chromium.org/140069
--~--~-~--
Thinking a bit more about interceptors modifying the objects. I am
not sure that should be allowed (meaning have well specified
behaviour). For example, AFAIK we'll notice updates to the objects
down the prototype chain, but not for objects up the prototype chain
(and it'd be somewhat cumbersome
Sorry for bothering you once again.
So before I finished this CL, I did
http://codereview.chromium.org/147021 (it didn't give a notable speed
up, so I didn't send it for review). However the idea is a pretty
close to how this CL under review should look like. Do you think
147021 is a reasonable
On Tue, Jun 23, 2009 at 1:00 PM, wrote:
> Hi Anton, this looks like a good start.
>
> Avoiding the extra lookups post interceptor looks like a good thing.
>
> Kasper just realized that what we have now is actually not safe. There
> is nothing that prevents the interceptor from adding or removing
Hi Anton, this looks like a good start.
Avoiding the extra lookups post interceptor looks like a good thing.
Kasper just realized that what we have now is actually not safe. There
is nothing that prevents the interceptor from adding or removing
properties from the object itself or any object in
Guys, that's a preliminary to get your feedback. If you like overall
approach, I'd implement it for ARM and clean up now unused stuff w/
lookup hints.
And if Kasper has some spare cycles, maybe we could try evolve this CL
into one not using trampoline at all (or might postpone that for
later).
29 matches
Mail list logo
