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 approach (modulo not checking prototype chain)? I'm esp. interested if handling of handles with Immediate is safe (meaning those survive GC)?
We could still want to pass LookupResult into interceptors though: I'm toying an idea to use those lookup results in interceptors which check if there are overrides (common case in some DOM bindings). tia and yours, anton. On Tue, Jun 23, 2009 at 5:46 PM, Anton Muhin<[email protected]> wrote: > On Tue, Jun 23, 2009 at 1:00 PM, <[email protected]> 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 >> properties from the object itself or any object in the prototype chain. >> Therefore, the cached index (or with this change lookup result) might >> not be valid when we get back from the interceptor. > > Awesome catch---tnx a lot! I've planned to separate checks for fully > inlined version, but didn't realized, it breaks the things. Guys, may > anyone of you help me with handles in asm? My understanding is as we > invoke interceptor, we might start GC, correct? > > And I'd address the rest of your notes, Mads, after reworking the whole CL. > > yours, > anton. > >> >> The right fix for this seems to be to continue working on this patch and >> modify it to: >> >> 1. check the maps to the interceptor holder >> >> 2. call the interceptor >> >> 3. recheck the entire map chain to the holder of the real property >> >> 4. call the runtime system to get the real property given the cached >> lookup result (which is valid since we have rechecked all the maps after >> calling the interceptor). >> >> This way it will be safe and we will not add extra overhead for the >> initial interceptor call and should still get a speedup for the >> subsequent lookup. >> >> >> http://codereview.chromium.org/140069/diff/1/2 >> File src/ia32/stub-cache-ia32.cc (right): >> >> http://codereview.chromium.org/140069/diff/1/2#newcode324 >> Line 324: static JSObject* Holder(JSObject* holder, LookupResult* >> lookup) { >> The argument here is the receiver, right? Rename |holder| argument to >> |receiver|. >> >> http://codereview.chromium.org/140069/diff/1/2#newcode329 >> Line 329: Object* current = holder; >> Can this be written in the style: >> >> for (Object* pt = current->GetPrototype(); >> pt != Heap::null_value(); >> pt = JSObject::cast(pt)->GetPrototype()) { >> ... >> } >> >> This is what we normally do. When does GetPrototype return undefined? >> >> http://codereview.chromium.org/140069/diff/1/2#newcode352 >> Line 352: void Compile(JSObject* holder, MacroAssembler* masm) { >> |holder| is unused? >> >> How about renaming to something like PushLookupResult? >> >> http://codereview.chromium.org/140069/diff/1/2#newcode374 >> Line 374: LookupResult* lookup, >> rename |lookup| to something like |post_interceptor_lookup|? >> >> http://codereview.chromium.org/140069/diff/1/2#newcode405 >> Line 405: if (lookup->IsValid()) { >> We should be able to combine the push of the real holder with the push >> of the rest of the lookup result in the helper class. >> >> http://codereview.chromium.org/140069/diff/1/2#newcode409 >> Line 409: LoadInterceptorHelper c(lookup); >> Rename c to helper? >> >> http://codereview.chromium.org/140069/diff/1/2#newcode416 >> Line 416: __ TailCallRuntime(load_ic_property, c.GetNArguments()); >> How about '4 + helper.LookupSize()' so that the helper only has to take >> the actual lookup result into account? >> >> http://codereview.chromium.org/140069/diff/1/2#newcode1079 >> Line 1079: &lookup, >> Pass in the name here and put the LoadInterceptorHelper in the generate >> method as is done for CallIC? >> >> http://codereview.chromium.org/140069/diff/1/2#newcode1207 >> Line 1207: &lookup, >> Ditto. >> >> http://codereview.chromium.org/140069 >> > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
