Closed Bug 458271 Opened 16 years ago Closed 10 years ago

Property cache causes resolve hooks not to be called

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
blocking2.0 --- -

People

(Reporter: jorendorff, Assigned: jorendorff)

Details

Attachments

(2 files)

v1
5.10 KB, patch
brendan
: review+
gal
: review-
Details | Diff | Splinter Review
resolver shell built-in, v1
7.20 KB, patch
brendan
: review+
Details | Diff | Splinter Review 
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.)
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.
Flags: blocking1.9.1+
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.
Flags: blocking1.9.1+ → blocking1.9.1-
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 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.
Assignee: general → jorendorff
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
blocking2.0: --- → beta1+
blocking2.0: beta1+ → beta2+
blocking2.0: beta2+ → betaN+
Attached patch v1 — — Splinter Review 
I think this fixes all the cases.
Attachment #463660 - Flags: review?(brendan)
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
Attachment #463660 - Flags: review?(brendan) → review+
Oh cool -- you get to remove the FIXME just added in the patch for bug 584565.

/be
http://hg.mozilla.org/tracemonkey/rev/96ef97b766e6

(Noted both places per comment 7.)
Whiteboard: [fixed-in-tracemonkey]
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.
Whiteboard: [fixed-in-tracemonkey]
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...
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?
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
(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 on attachment 463660 [details] [diff] [review]
v1

resolve can be JS_ResolveStub here, not just NULL.
Attachment #463660 - Flags: review-
(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
(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.
Even with the JS_ResolveStub thing fixed, this is a 7% loss on SunSpider and a 17% loss on V8.
(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.
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?
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).
(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
Just talked to Brendan, he doesn't think this is likely to hurt the web, so not a priority for now.
blocking2.0: betaN+ → -
> 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?
(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.
(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.
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);
Attachment #513162 - Flags: review?(brendan)
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
Attachment #513162 - Flags: review?(brendan) → review+
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.
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.)
http://hg.mozilla.org/mozilla-central/rev/767af66d704c
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Should I have jsfunfuzz call the resolver() function added above?
Give it a try. I think we should be pretty robust in this area.
WFM on Beta and Nightly.
I always get "object" on the test from comment 0.
Can anyone confirm?
The propcache is long gone.
(Can't find the bug about propcache removal, so WFM)
Status: REOPENED → RESOLVED
Closed: 13 years ago → 10 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: