416931 - XPC_WN_Helper_SetProperty overwhelms property-set performance, causes global warming

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

In sunspider:bitops-bitwise-and, we see 75% of our time spent in the XPConnect property machinery.

We hit the L2 propcache (sprop case) when setting a window global that's not a var, but then we have a custom getter/setter that dives down into nsWindow and nsEventHandlerSH to make sure we're not setting a magic property like onfoo or location.

I propose that we teach XPConnect to use a custom lookup op which can return a basic slot-and-stubs sprop for these non-magic cases, such that they go like stink.

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

We could whittle off some overhead if we got rid of the request in nsEventHandlerSH::SetProperty, but I'd rather go for the gold here.

Comment 2

[Johnny Stenback (:jst)]

Taking.

Comment 3

[Johnny Stenback (:jst)]

Attachment: Make expandos fast!

This speeds up global expandos. Basically for all property sets that make it down to the end of nsWindowSH::SetProperty() we know they're expandos, properties that are not defined in IDL. That means global variables on web pages, global functions and the like. This code basically just redefines those properties with a property with the same value and js_PropertyStub as the getter/setter. This way we don't go through XPConnect at all for these properties any more. Ideally we'd be able to do this in the resolve hook, but testing showed that it's near impossible to get this right there due to things happening really early on during initialization etc. I never did track down the exact reason, but it seems to only make sense to do this in the set property hook, as there we already know that the property is an expando.

W/o this patch I get ~815ms for the bitwise and test, with this patch I get ~360ms, so 2x+!

Oh, and this is all possible thanks to mrbkap's XOW work, before that we *had* to go through the get/set property hooks to do the necessary security checks. Now the XOWs take care of that and the security checks are a thing of the distant past.

Comment 4

[Johnny Stenback (:jst)]

Oh, and I forgot to mention that the correctness issue I was worried about with this patch (mentioned in an email thread about this) was a false assumption due to a bug in a test I had. My worry was that creating an <iframe name="foo"> would override window.foo to point to the iframe even if window.foo had been set before the frame was even created, turns out not to be true. What we *must* continue to support (which my patch does) is that a window.foo property that references an iframe must be updated if the iframe is replaced with another iframe with the same name. So the patch is good in that sense AFAICT.

Comment 5

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 302956 [details] [diff] [review]
Make expandos fast!

This patch wants JS_DefineUCPropertyById with a jsval id in place of name, but since we don't _have_ that, I'll let it go.  We're not going to optimize window[0] in this case, though. :) r+sr=shaver

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Should we do this on other nodes as well, to make their expandos fast?  I mean, you know, while we're rocking it.

Comment 7

[Johnny Stenback (:jst)]

Fix checked in, and yeah, optimizing window[n] where n is an integer probably isn't worth it (though we could do that too with JS_DefineElement() if we *really* thought it was worth the extra code).

Marking FIXED.

Comment 8

[Johnny Stenback (:jst)]

(In reply to comment #6)
> Should we do this on other nodes as well, to make their expandos fast?  I mean,
> you know, while we're rocking it.

Sorry, missed this comment. We *could* do that too, but I personally doubt it'll have much of a real-world impact. If there's testcases that make nsNodeSH::SetProperty() stand out somewhere, I'd gladly reconsider.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Could we add a test for the iframe correctness issues (both of them) mentioned in comment 4?  As well as a test for this bug itself, if we can?

Comment 10

[Ria Klaassen (not reading all bugmail)]

Could this bug have caused a very high memory and CPU usage when trying to access http://docs.google.com/#all ?

Comment 11

[Walter Rimini]

Same happens in http://www.google.com/reader/view/#.
Tested with a new profile, no extensions. Previous hourly works fine.
Memory usage goes from 40Mb to over 400mb, and then back to 40mb after page finishes loading.

Comment 12

[Johnny Stenback (:jst)]

This bug is indeed to blame for that, reopening. Doing this in the setProperty() hook makes the property cache blow up on us. The google reader page ends up defining ~2600 global properties that go through this path, and because we mutate the property cache while a property is being set, it blows up on us. I'm going to take another look at doing this from resolve() as I first attempted to.

Comment 13

[Brendan Eich [:brendan]]

s/property cache/property tree/ in comment 12 -- the tree is a lexicographic tree of structural types ("object shapes"), so for a global object with 26 properties, named A through Z with certain attributes, getters, setters, etc., if you redefine A to have stub getter and setter, the entire ancestor line from Z up to A that defines the shape of the global object must be forked to A' through Z' (in case other objects are sharing the original Z up to A shape).

I should have seen this coming. It's a hard case, atypical and hard to do via the JS language but possible via the JS API as this bug shows. We should try to use the resolve hook as intended, and avoid redefining properties high in the property tree.

/be

Comment 14

[Boris Zbarsky [:bzbarsky]]

Won't using the resolve hook break situations where someone extends Window.prototype?  The resolve will resolve all properties on the object itself, so the prototype will never get looked at...

Unless we'll put a nested resolve on the proto in the window resolve?

Comment 15

[Brendan Eich [:brendan]]

Can OBJ_LOOKUP_PROPERTY on the prototype of obj to handle that. Or, here's a better idea: if js_CheckRedeclaration doesn't resolve a built-in property, then the OBJ_DEFINE_PROPERTY in the JSOP_DEF{CONST,VAR} case in js_Interpret should use JS_PropertyStub for the getter and setter.

In other words, for a global variable that's not built-in, a true expando, we use the getter and setter stubs. The embedding loses its chance to intercept gets and sets on such properties by not resolving them. I'll put up a patch.

/be

Comment 16

[Brendan Eich [:brendan]]

Attachment: JS interpreter fix

jst, can you try this out? Thanks,

/be

Comment 17

[Brendan Eich [:brendan]]

Also need a back-out of the first patch -- jst, can you do that deed? Want to keep this bug validly reopened, and patched once.

/be

Comment 18

[Brendan Eich [:brendan]]

Attachment: cover the JSOP_GETTER/SETTER cases too

The OBJ_DEFINE_PROPERTY protocol is if (attrs & JSPROP_GETTER), then the getter arg is a (JSObject*) and it should override any prior getter if the id'd property exists, else make a new one with that getter function; ditto JSPROP_SETTER. This supports composing the two halves of a get/set pair via an object initialiser:

var obj = { get x() ++this._x, set x(y) this._x = y };

As elsewhere in the patch, when the get is processed, 'x' is bound to the given getter function distinguished by JSPROP_GETTER, and JS_PropertyStub with no JSPROP_SETTER attribute. Then the set is processed, finding 'x' and filling in the setter function and setting JSPROP_SETTER.

Same goes for __defineGetter__ and __defineSetter__, so this patch touches jsobj.c too.

If only one half of the pair is defined, the other should remain a non-function (plain old JSPropertyOp native) stub getter or setter: JS_PropertyStub. In the case of a property with only JSPROP_GETTER, an attempt to set such a property is a TypeError. A property with only JSPROP_SETTER can be read from via its plain old getter, which this patch makes JS_PropertyStub -- it would be wrong for the class getProperty hook to run instead, since the getter-less setter has no slot.

In fact all user-defined getters and setters are slot-less (JSPROP_SHARED), so at the limit not usable by the class getProperty hook, which would expect to handle slotful properties that lack more specific getters.

This fix is a change to API semantics, though I think it's an improvement. It also gives us universal property caching for global vars and functions that are ad-hoc user-defined expandos, not builtins. It passes the JS testsuite.

/be

Comment 19

[Robert Sayre]

Comment on attachment 303663 [details] [diff] [review]
cover the JSOP_GETTER/SETTER cases too

This works

Comment 20

[Johnny Stenback (:jst)]

Brendan, I haven't had a chance to test your patches yet, but wouldn't they make it such that XPConnect can't detect property sets in the cases where it currently prevents setting expandos on wrappers (i.e. the xpcwrappednativejsops.js hits at http://lxr.mozilla.org/mozilla/search?string=XPC_CANT_MODIFY_PROP_ON_WN)?

Comment 22

[Brendan Eich [:brendan]]

jst: seems like addProperty should be enough -- if addProperty fails, there'll be no sets. Not sure why jband stubbed both with the CannotModify or OnlyIWrite kind of stubs.

Could you back out the nsDOMClassInfo change since we are collecting dups? I'd do it but I've got my own regressions to atone for.

/be

Comment 23

[Brendan Eich [:brendan]]

jst: if you could test, that would be awesome. If as I hope, failing addProperty suffices, then the setProperty class hooks that XPConnect assigns to the same CannotModify and OnlyIWrite stubs as the addProperty ones probably should be JS_PropertyStub -- or perhaps an assertion that the setProperty hooks are not called.

I hope XPConnect is not using the same stub for add and set because it needs to allow the add but fail a subsequent set, depending on phase of moon etc. That would frustrate my hopes. Quick reading does not support that -- what do you think?

/be

Comment 24

[Robert Sayre]

(In reply to comment #23)
> jst: if you could test, that would be awesome.

Add test cases to the bug--I'll get them into mochitest.

Comment 26

[Johnny Stenback (:jst)]

Original + followup changes backed out.

Comment 27

[Brendan Eich [:brendan]]

jst: thanks! Sorry I didn't cut to the interpreter-level-fix chase sooner. Have you been able to verify that addProperty checking is enough?

/be

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 303663 [details] [diff] [review]
cover the JSOP_GETTER/SETTER cases too

Going for gold here...

/be

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 303663 [details] [diff] [review]
cover the JSOP_GETTER/SETTER cases too

I can't think of any cases where this should change behavior. jst should speak up if he can.

Comment 30

[Brendan Eich [:brendan]]

I committed the patch:

js/src/jsinterp.c 3.430
js/src/jsobj.c 3.436

Let's see what breaks, if anything.

/be

Comment 31

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Did anyone ever get test cases for this into mochi?  Sayrer made the kind offer of carrying things from a snippet, back in comment 24, but nobody has provided a test for him to use, that I can see.

Is it enough to just set a few thousand global properties?  How would we mochi for performance degradation?  Measure 500 prop case, then measure 5000 prop case, and see if the performance is roughly linear?  :mystery:

Comment 32

[Brendan Eich [:brendan]]

Running bitwise-and shows the difference, doesn't it? jst said twice as slow, IIRC.

/be