Closed Bug 416931 Opened 12 years ago Closed 12 years ago

XPC_WN_Helper_SetProperty overwhelms property-set performance, causes global warming

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: shaver, Assigned: brendan)

References

Details

(Keywords: perf)

Attachments

(2 files, 1 obsolete file)

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.
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.
Taking.
Assignee: nobody → jst
Keywords: perf
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.
Attachment #302956 - Flags: superreview?(shaver)
Attachment #302956 - Flags: review?(shaver)
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.
Depends on: xow
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
Attachment #302956 - Flags: superreview?(shaver)
Attachment #302956 - Flags: superreview+
Attachment #302956 - Flags: review?(shaver)
Attachment #302956 - Flags: review+
Flags: blocking1.9+
Should we do this on other nodes as well, to make their expandos fast?  I mean, you know, while we're rocking it.
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Priority: -- → P1
Resolution: --- → FIXED
(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.
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?
Flags: in-testsuite?
Depends on: 417266
Could this bug have caused a very high memory and CPU usage when trying to access http://docs.google.com/#all ?
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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
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?
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
Attached patch JS interpreter fix (obsolete) — Splinter Review
jst, can you try this out? Thanks,

/be
Assignee: jst → brendan
Status: REOPENED → ASSIGNED
Attachment #303656 - Flags: review?(igor)
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
Target Milestone: --- → mozilla1.9beta4
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
Attachment #303656 - Attachment is obsolete: true
Attachment #303656 - Flags: review?(igor)
Comment on attachment 303663 [details] [diff] [review]
cover the JSOP_GETTER/SETTER cases too

This works
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)?
Duplicate of this bug: 417536
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
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
(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.
Duplicate of this bug: 417966
Original + followup changes backed out.
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
Component: XPConnect → JavaScript Engine
QA Contact: xpconnect → general
Comment on attachment 303663 [details] [diff] [review]
cover the JSOP_GETTER/SETTER cases too

Going for gold here...

/be
Attachment #303663 - Flags: review?(mrbkap)
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.
Attachment #303663 - Flags: review?(mrbkap) → review+
I committed the patch:

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

Let's see what breaks, if anything.

/be
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Blocks: 417327
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:
Running bitwise-and shows the difference, doesn't it? jst said twice as slow, IIRC.

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