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. 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 -~----------~----~----~----~------~----~------~--~---
