Closed
Bug 517143
Opened 15 years ago
Closed 7 years ago
Try to wean domclassinfo off class getters/setters
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Unassigned)
References
(Blocks 2 open bugs)
Details
(Keywords: perf)
We don't trace sets of properties that have a slot _and_ a setter. The problem is that this is what all DOM expandos look like right now. Ideally we would wean nsDOMClassInfo off using class getters/setters at least for nodes and documents. This should be doable by having resolve hooks define JSPropertyOp getters/setters as needed. I was going along swimmingly on making that change, but then I realizes that we have scriptable hook callers other than the class ops in xpcwrappednativejsops; in particular we have XPCWrapper::GetOrSetNativeProperty. This is what lets .onfoo work for an XPCNativeWrapper, for example. At this point, the options as I see them are: 1) Change XPCWrapper somehow to deal with the new world in which random classinfo-defined props look just like expandos. Since XPCNativeWrapper is not supposed to hand back expandos, this might be hard. 2) Change xpcwrappednativejsops to make the USE_JSSTUB_FOR_SETPROPERTY flag override the WANT_SETPROPERTY flag. Make sure the former is unset anywhere we really care about getting our setproperty called. Leave getters as they are now (so random expandos on DOM nodes would have a getter). Any other options? Option 1 sounds like a pain. Option 2 works for setters, but not really getters... and is an API change (granted, this is a so-totally-not-frozen API).
Reporter | ||
Comment 1•15 years ago
|
||
mrbkap suggests that we just nix the WANT_* flags, have the resolve hooks define the props on the _wrapper_ as needed, and have some magic in the wrapper's AddProperty or something to rewrap those hooks in the wrapper's own "I'll call this thing and then wrap the result" hooks. Or something.
Reporter | ||
Comment 2•15 years ago
|
||
Note that just making this precise testcase trace without changing classinfo would likely leave this issue: trace stopped: 12406: can't trace through access to undefined property if JSClass.getProperty hook isn't stubbed Abort recording of tree file:///Users/bzbarsky/prototype.js:3313@65 at file:///Users/bzbarsky/prototype.js:3314@66: getlocalprop.
Reporter | ||
Comment 3•15 years ago
|
||
OK, so I just had a nice long conversation with Blake. Current plan: Classinfo nixes the WANT_GET/SET* flags. We use the resolve hook to set up properties with custom getters/setters. Either the resolve hook detects that the object is a wrapper and sets up a getter/setter that calls the "normal" one and then rewraps as needed, or it always sets up the same getter/setter and that does the detection on each get/set. I'd somewhat prefer the former. We remove the classinfo bits from the wrapper getters/setters, but obviously keep the classinfo resolve call. All we need now is someone to implement....
Comment 4•15 years ago
|
||
Peter, you had a plan to eliminate some of our resolve hooks in classinfo for other performance reasons, sounds like your plan and bz/mrbkap's plan are likely conflicting here...
Reporter | ||
Comment 5•15 years ago
|
||
To be clear, the plan from comment 3 doesn't _add_ any resolve hooks. We already have resolve hooks for all the cases covered by the classinfo getters/setters, as far as I can tell. So I suspect the two plans are quite compatible.
Comment 6•15 years ago
|
||
Yeah, I think there won't be a conflict.
Comment 8•13 years ago
|
||
How does this bug differ from bug 516857?
Reporter | ||
Comment 9•13 years ago
|
||
Bug 516857 is for the first set of an expando on a given object, which at the time trace-aborted. After that setting _other_ expando properties (with different names!) on the same object could happen on trace. This bug is about the fact that setting the same expando property, with the _same_ name, on an object multiple times fails to trace. And also getting expandos fails. And JM ICs for expandos on DOM nodes fail.
Comment 10•13 years ago
|
||
bz, is this valid as a non-TM bug?
Reporter | ||
Comment 11•13 years ago
|
||
That's a good question. We got rid of some of these getters/setters in bug 660233, bug 659350, bug 639720, because they were affecting JM ICs. As this point, most DOM nodes do not have a classinfo getter or setter. Furthermore, JM may now be able to IC these class getters/setters, with Brian's recent changes. Not sure about that. But for bug 580070 we might need more work here...
Reporter | ||
Updated•13 years ago
|
Blocks: ParisBindings
Comment 12•11 years ago
|
||
With bug 823223 fixed, anything else to do here?
Reporter | ||
Comment 13•11 years ago
|
||
We still have users of this stuff in classinfo. Specifically, Storage, History, CSSRuleList, StyleSheetList, DOMStringList. Not sure any of these are perf-sensitive, which is what this bug was about. And the right way to deal with them is to switch to WebIDL bindings.
Reporter | ||
Comment 14•8 years ago
|
||
This is fixed. The only users of WANT_GET/SETPROPERTY are in places code now...
Comment 15•7 years ago
|
||
I think this was fixed by bug 1389510.
Reporter | ||
Comment 16•7 years ago
|
||
It was fixed a long time ago by webidl bindings; see comment 14.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•