Closed Bug 517143 Opened 11 years ago Closed 2 years ago

Try to wean domclassinfo off class getters/setters

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Unassigned)

References

(Blocks 3 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).
Blocks: 517636
Keywords: perf
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.
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.
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....
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...
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.
Yeah, I think there won't be a conflict.
Blocks: 559396
This is causing us slow stub calls with JM too.
Blocks: domperf
How does this bug differ from bug 516857?
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.
bz, is this valid as a non-TM bug?
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...
Depends on: 660233, 659350, 639720
Summary: Setting expandos on DOM nodes repeatedly doesn't trace; try to wean domclassinfo off class getters/setters → Try to wean domclassinfo off class getters/setters
Depends on: 823223
With bug 823223 fixed, anything else to do here?
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.
This is fixed.  The only users of WANT_GET/SETPROPERTY are in places code now...
I think this was fixed by bug 1389510.
It was fixed a long time ago by webidl bindings; see comment 14.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.