Last Comment Bug 458271 - Property cache causes resolve hooks not to be called
: Property cache causes resolve hooks not to be called
Status: RESOLVED WORKSFORME
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jason Orendorff [:jorendorff]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-10-02 14:12 PDT by Jason Orendorff [:jorendorff]
Modified: 2014-04-14 02:57 PDT (History)
16 users (show)
shaver: blocking1.9.1-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
v1 (5.10 KB, patch)
2010-08-06 14:31 PDT, Jason Orendorff [:jorendorff]
brendan: review+
gal: review-
Details | Diff | Splinter Review
resolver shell built-in, v1 (7.20 KB, patch)
2011-02-17 09:54 PST, Jason Orendorff [:jorendorff]
brendan: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2008-10-02 14:12:56 PDT
We currently fill the property cache even if js_LookupPropertyWithFlags called resolve hooks.  This means that later, the same lookup will not call resolve hooks.  For example,

<!DOCTYPE HTML>
<body>
<form name=f></form>
<script>
var f = document.forms.f;
function lookup() {
    return f.target;
}

f.innerHTML = '';  // make sure later f.innerHTML doesn't change shape
lookup();
lookup();          // nsIDOMHTMLFormElement.target goes into propcache
f.innerHTML = '<input type=text name=target>';  // should shadow f.target
alert(typeof lookup());   // resolve hook isn't called; get wrong f.target
</script>

When I reload this repeatedly, sometimes I get "object" and sometimes "string".  I think it should always say "object".

(This bug makes me sad, because if we fix it, the resolve hooks make it harder for me to JIT calls into the DOM.)
Comment 1 Jason Orendorff [:jorendorff] 2008-10-02 20:58:00 PDT
There could be a context option, JSOPTION_CACHE_RESOLVE_MISSES.  When it's off, we call the resolve hook every time.  When it's enabled, we behave like we do now.  The application can invalidate the cache by defining a property.  Form elements would have to do this when mutated.
Comment 2 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-11-26 09:28:17 PST
I propose that this not block 1.9.1, since we won't be JITting DOM calls, and it's not a regression (even for SM embedders) versus 3.0.  Renom if you disagree, by all means.
Comment 3 Brendan Eich [:brendan] 2010-03-24 13:42:31 PDT
This bug needs an owner. It seems like it could be causing ongoing undiagnosed bugs, but maybe it's less severe. Hard to say. Jason, what do you think?

/be
Comment 4 Jason Orendorff [:jorendorff] 2010-04-15 07:08:37 PDT
Comment 1 is silly.

An easy fix is to detect resolve hooks at record time and emit a slower path that just calls the appropriate op. We already have these slow paths, for GETELEM and friends.

In the browser, the global object has an expensive resolve hook. But it shouldn't be a performance problem, because if we end up running into it at record time, that means either (1) the resolve hook will define a property, which would change the global shape and bump us off trace anyway; or (2) it won't: the property doesn't exist. In case 2, what instruction are we executing? JSOP_NAME/CALLNAME/GETXPROP/INCNAME will throw (unless we happen to find the property on Object.prototype). JSOP_BINDNAME/SETNAME will end up changing the global shape. So emitting a slow path here is no big deal--and maybe we should even abort.
Comment 5 Brendan Eich [:brendan] 2010-04-15 13:23:20 PDT
We import globals, treat them as locals if you will. JaegerMonkey wants to do this too.

One issue with the global object not resolving an id is that an object higher up the prototype chain might. But I think this is true only for IE-emulating doc.all type name polluter objects and such. Cc'ing jst and mrbkap.

Hope we can just abort and get on with our lives, with no new aborts on hot perf tests and real apps.

/be
Comment 6 Jason Orendorff [:jorendorff] 2010-08-06 14:31:19 PDT
Created attachment 463660 [details] [diff] [review]
v1

I think this fixes all the cases.
Comment 7 Brendan Eich [:brendan] 2010-08-06 14:49:25 PDT
Comment on attachment 463660 [details] [diff] [review]
v1

This all looks great. As noted on IRC, fixing bug 506341 will make this test fail only sporadically if this bug's fix is regressed. Note that somewhere (maybe in two places, bug 506341 and in a comment in the test)?

/be
Comment 8 Brendan Eich [:brendan] 2010-08-06 15:20:39 PDT
Oh cool -- you get to remove the FIXME just added in the patch for bug 584565.

/be
Comment 9 Jason Orendorff [:jorendorff] 2010-08-07 13:18:46 PDT
http://hg.mozilla.org/tracemonkey/rev/96ef97b766e6

(Noted both places per comment 7.)
Comment 10 Robert Sayre 2010-08-07 20:14:34 PDT
I backed this out because it was failing JIT stats. I couldn't tell if they were legit.

http://hg.mozilla.org/tracemonkey/rev/8292348dc597

don't h9 me.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2010-08-07 22:19:28 PDT
I should note that while the patch was in it was a 3x (as in, 38 instead of 121 as the score) slowdown on the dromaeo DOM traversal tests over here, as far as I can tell...
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2010-08-07 22:43:12 PDT
And in particular, if resolve hooks become a serious performance hit (which they do with this patch) then we need to figure out how to stop using them in classinfo.  But we also want to stop using getter/setter hooks...  So we really just need a different approach to reflecting the DOM into JS altogether, or something.

Could we have a variant of a resolve hook that promises to observe certain rules that would make it idempotent, perhaps?
Comment 13 Brendan Eich [:brendan] 2010-08-07 23:21:02 PDT
Re: comment 0 testcase:

> f.innerHTML = '<input type=text name=target>';  // should shadow f.target

Shadowing purges by reshaping shadowed properties' objects. That does not happen if resolve is not run by PurgeProtoChain (it's not).

But are we even calling js_PurgeScopeChain when that name=target is processed?

The fundamental bug here has not been fixed. I should have dug deeper, since in general the comment from the patch "The non-natives can mutate in arbitrary way without changing any shapes" is true of "host objects" -- but our DOM objects are not so badly behaved.

Seems to me the fault lies not with the host object stars but with ourselves (JS engine not resolving when purging, or something akin).

/be
Comment 14 David Anderson [:dvander] 2010-08-08 15:19:52 PDT
(In reply to comment #11)

From looking at regress-x86 on graphs this was a ~4X perf regression on shell v8 before the backout.
Comment 15 Andreas Gal :gal 2010-08-09 14:55:22 PDT
Comment on attachment 463660 [details] [diff] [review]
v1

resolve can be JS_ResolveStub here, not just NULL.
Comment 16 Brendan Eich [:brendan] 2010-08-09 18:43:24 PDT
(In reply to comment #15)
> Comment on attachment 463660 [details] [diff] [review]
> v1
> 
> resolve can be JS_ResolveStub here, not just NULL.

D'oh -- I was dreaming that the JSObjectOps -> JSClass coalescing had changed maybe-stubbing to use null only.

/be
Comment 17 Igor Bukanov 2010-08-10 06:16:10 PDT
(In reply to comment #16)
> D'oh -- I was dreaming that the JSObjectOps -> JSClass coalescing had changed
> maybe-stubbing to use null only.

This still can be done - see bug 585930.
Comment 18 Jason Orendorff [:jorendorff] 2010-08-17 10:59:07 PDT
Even with the JS_ResolveStub thing fixed, this is a 7% loss on SunSpider and a 17% loss on V8.
Comment 19 Jason Orendorff [:jorendorff] 2010-08-17 11:08:33 PDT
(In reply to comment #13)
> Re: comment 0 testcase:
> > f.innerHTML = '<input type=text name=target>';  // should shadow f.target
> 
> Shadowing purges by reshaping shadowed properties' objects. That does not
> happen if resolve is not run by PurgeProtoChain (it's not).
> 
> But are we even calling js_PurgeScopeChain when that name=target is processed?

No, because although they're all native objects, processing name=target does not cause the .target property to be added right away. No JSObject is touched. The resolve hook is supposed to do that later.

The bug here is that the resolve hook never gets called again, so the property is never created at all.

> The fundamental bug here has not been fixed. I should have dug deeper, since in
> general the comment from the patch "The non-natives can mutate in arbitrary way
> without changing any shapes" is true of "host objects" -- but our DOM objects
> are not so badly behaved.

They are badly-behaved enough.
Comment 20 Jason Orendorff [:jorendorff] 2010-08-17 11:16:40 PDT
The SunSpider slowdown is almost entirely due to date-format-xparb.js and string-validate-input.js, which use inherited properties/methods of String and RegExp objects, respectively. Both of those classes have resolve hooks. So does Function.

I could special-case those internal hooks or tweak the JSNewResolveHook contract to allow the hook to say whether the result is cacheable. Thoughts?
Comment 21 Jason Orendorff [:jorendorff] 2010-08-17 12:03:35 PDT
The V8 slowdown is due to the same sort of thing (in v8-deltablue, myfunction.call() triggers the Function resolve hook, so with the patch, we don't try to trace it).
Comment 22 Brendan Eich [:brendan] 2010-08-17 17:09:23 PDT
(In reply to comment #19)
> (In reply to comment #13)
> > Re: comment 0 testcase:
> > > f.innerHTML = '<input type=text name=target>';  // should shadow f.target
> > 
> > Shadowing purges by reshaping shadowed properties' objects. That does not
> > happen if resolve is not run by PurgeProtoChain (it's not).
> > 
> > But are we even calling js_PurgeScopeChain when that name=target is processed?
> 
> No, because although they're all native objects, processing name=target does
> not cause the .target property to be added right away. No JSObject is touched.
> The resolve hook is supposed to do that later.
> 
> The bug here is that the resolve hook never gets called again, so the property
> is never created at all.

The theory was the property cache didn't need to call resolve hooks because the lack of a resolve hook call that actually defined a new property would guarantee a miss due to shapes differing somewhere.

The testcase in comment 0 shows this is not valid, because resolve is passive and the reshaping is entirely hidden from the JS object system. But should it be?

With no shape change anywhere when f.innerHTML is set, we are bound to get a stale hit on the prototype (string-valued) target property. So let's try to fix the bug by asserting that setting f.innerHTML should change some shape.

1. nsIDOMHTMLFormElement.prototype's shape should change by eagerly evaluating the inner HTML and binding f.target, therefore purging the proto chain and reshaping nsIDOMHTMLFormElement.prototype; or

2. f's shape and every one of its prototype chain elements' shapes should be regenerated by fiat because it wants to be lazy but cannot rule out some lazy evaluation of its new inner HTML shadowing something.

Presumably 1 is too expensive. How about 2?

Jason, other examples like this innerHTML-based one? It is nasty!

I'm loath to try whitelisting resolve hooks but we could go there. I bet the list will grow quite large.

/be
Comment 23 David Mandelin [:dmandelin] 2010-12-09 17:57:43 PST
Just talked to Brendan, he doesn't think this is likely to hurt the web, so not a priority for now.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2010-12-31 23:37:41 PST
> Presumably 1 is too expensive. How about 2?

To make this work, we would need to, for every DOM mutation, look for a <form> on the parent chain of the mutation and reshape it.  Or something like that.

How about we instead fix the DOM to do the NameGetter thing, so there aren't real properties corresponding to the things involved here?  If we do it right, things should hopefully Just Work, right?
Comment 25 Jason Orendorff [:jorendorff] 2011-02-17 07:27:51 PST
(In reply to comment #22)
> Jason, other examples like this innerHTML-based one? It is nasty!

Bug 633890 provides this simpler example:

<!doctype html>
<form></form>
<form><input name="id"></form>
<script>
var a = [document.forms[0], document.forms[1]], v;
for (var i = 0; i < 2; i++)
    v = a[i].id;
alert(v === document.forms[1].id ? "PASS" : "FAIL");
</script>

> I'm loath to try whitelisting resolve hooks but we could go there. I bet the
> list will grow quite large.

Instead of centrally whitelisting, we could let each JSClass with a well-behaved resolve hook opt in using a new JSClass.flags bit, JSCLASS_CACHEABLE_RESOLVE.
Comment 26 Igor Bukanov 2011-02-17 08:07:38 PST
(In reply to comment #25)
> Instead of centrally whitelisting, we could let each JSClass with a
> well-behaved resolve hook opt in using a new JSClass.flags bit,
> JSCLASS_CACHEABLE_RESOLVE.

What about introducing a new resolve hook that would return a property descriptor for the new property with a requirement that it must be well-behaved where "well-behaved" means not altering the object in any way? Compared with JSCLASS_CACHEABLE_RESOLVE it would require to change the hook code so we would know that we have not missed non-cachable cases. 

That new resolve hook could be even a static array of property descriptors that can be quickly searched without any hook invocation. Given that we are executing about 15000 resolve hooks on startup, it may help to reduce our startup overhead.
Comment 27 Jason Orendorff [:jorendorff] 2011-02-17 09:54:47 PST
Created attachment 513162 [details] [diff] [review]
resolver shell built-in, v1

I'd like to land this. It adds a builtin function, resolver, to the JS shell. With this function, the test above can be written in the shell:

var p = {x: 0};
var a = [resolver({}, p), resolver({x: 1}, p)];  // same shape
var v;
for (var i = 0; i < a.length; i++)
    v = a[i].x;
assertEq(v, 1);
Comment 28 Brendan Eich [:brendan] 2011-02-17 16:08:34 PST
Comment on attachment 513162 [details] [diff] [review]
resolver shell built-in, v1

>+        desc.getter = shape->getter();
>+        if (!desc.getter)
>+            desc.getter = PropertyStub;
>+        desc.setter = shape->setter();
>+        if (!desc.setter)
>+            desc.setter = StrictPropertyStub;

The desc.[gs]etter members need to be null if the corresponding JSPROP_[GS]ETTER is set in attrs.

>+        desc.attrs = shape->attributes();
>+        desc.shortid = shape->shortid; 
>+        propFlags = shape->getFlags();
>+   } else if (referent->isProxy()) {
>+        PropertyDescriptor desc;
>+        if (!JSProxy::getOwnPropertyDescriptor(cx, referent, id, false, &desc))

We so need ObjectOps to have getOwnPropertyDescriptor.

>+            return false;
>+        if (!desc.obj)
>+            return true;
>+    } else {
>+        if (!referent->lookupProperty(cx, id, objp, &prop))
>+            return false;
>+        if (*objp != referent)
>+            return true;
>+        if (!referent->getProperty(cx, id, &desc.value) ||
>+            !referent->getAttributes(cx, id, &desc.attrs)) {
>+            return false;
>+        }
>+        desc.getter = PropertyStub;
>+        desc.setter = StrictPropertyStub;

A non-native could have JSPROP_GETTER or JSPROP_SETTER set in its attrs, if it were evil. Clear those bits.

>+resolver_enumerate(JSContext *cx, JSObject *obj)
>+{
>+    jsval v;
>+    JS_ALWAYS_TRUE(JS_GetReservedSlot(cx, obj, 0, &v));
>+    JSObject *referent = JSVAL_TO_OBJECT(v);
>+
>+    JSIdArray *ida = JS_Enumerate(cx, referent);

OOM-check.

>+Resolver(JSContext *cx, uintN argc, jsval *vp)
>+{
>+    // First argument, object to lazily duplicate, is required.
>+    jsval v;
>+    if (argc < 1 || JSVAL_IS_PRIMITIVE(v = JS_ARGV(cx, vp)[0])) {
>+        JS_ReportError(cx, "resolver: object expected");
>+        return false;
>+    }
>+    JSObject *referent = JSVAL_TO_OBJECT(v);
>+
>+    // Second argument, prototype object, is optional. Default to Object.prototype.
>+    JSObject *proto = NULL;
>+    bool protoGiven = false;
>+    if (argc > 1) {
>+        if (!JSVAL_IS_OBJECT(v = JS_ARGV(cx, vp)[1])) {

Seems like JS_ConvertArguments would be shorter and sweeter here.

r=me with fixes.

/be
Comment 29 Jason Orendorff [:jorendorff] 2011-02-18 16:42:44 PST
Comment on attachment 513162 [details] [diff] [review]
resolver shell built-in, v1

(In reply to comment #28)
> We so need ObjectOps to have getOwnPropertyDescriptor.

Yes.

> A non-native could have JSPROP_GETTER or JSPROP_SETTER set in its attrs, if it
> were evil. Clear those bits.

True. I decided to whitelist instead of blacklisting. Now it allows only the attributes that make sense on plain old data properties: JSPROP_ENUMERATE, JSPROP_READONLY, and JSPROP_PERMANENT.

> >+    JSIdArray *ida = JS_Enumerate(cx, referent);
> 
> OOM-check.

This was also a GC hazard, so I changed it to use js::AutoIdArray and added a check.

Thanks.
Comment 30 Jason Orendorff [:jorendorff] 2011-02-21 09:08:37 PST
I pushed the resolver shell built-in, for easier testing of this bug.
http://hg.mozilla.org/tracemonkey/rev/767af66d704c

(Note: This bug still isn't fixed.)
Comment 32 Jesse Ruderman 2013-11-16 02:48:19 PST
Should I have jsfunfuzz call the resolver() function added above?
Comment 33 Jason Orendorff [:jorendorff] 2013-11-18 09:44:31 PST
Give it a try. I think we should be pretty robust in this area.
Comment 34 Guilherme Lima 2014-04-13 18:06:38 PDT
WFM on Beta and Nightly.
I always get "object" on the test from comment 0.
Can anyone confirm?
Comment 35 Boris Zbarsky [:bz] (still a bit busy) 2014-04-13 18:59:31 PDT
The propcache is long gone.
Comment 36 Till Schneidereit [:till] 2014-04-14 02:57:05 PDT
(Can't find the bug about propcache removal, so WFM)

Note You need to log in before you can comment on or make changes to this bug.