Closed Bug 297145 Opened 19 years ago Closed 7 years ago

investigate whether JS_GetProperty in nsDOMClassInfo::PostCreate is needed

Categories

(Core :: DOM: Core & HTML, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: bryner, Assigned: jst)

References

()

Details

(Keywords: helpwanted, perf)

jst and I are a bit baffled by what this code is trying to do. If we could get rid of it, I think it would speed up DOM traversal from script a bit.
So this looks like it's just checking that window.Node (or equivalent for other classes) is defined, right? That does seem pretty pointless to me.... unless this triggers our lazy global object class init?
In particular, see the work done in nsWindowSH::GlobalResolve... I guess the intent here could be that the Node class (say) is all correctly set up before we return a Node wrapper... If so, maybe there's a better way to do it? Say keeping some out-of-band state as to which classes are already ok?
::JS_LookupProperty suffices to trigger a resolve hook. ::JS_GetProperty does a lookup and if that succeeds, calls the property's getter -- which is not certain to be idempotent. Don't do unnecessary gets when lookups are enough. /be
I suspect the slow part here is getting the global object and making the call at all, not the specifics of what call we're making...
This code just got removd in bug 326497, but I'm still not sure whether that's ok (see comment 2).
Flags: blocking1.9a1?
So, it turns out that the GetProperty is needed for reflecting functions stuck on <classname>.prototype.
Do we know why?
I'm going to try to figure that out today or tomorrow.
Blocks: 326497
(In reply to comment #2) > In particular, see the work done in nsWindowSH::GlobalResolve... I guess the > intent here could be that the Node class (say) is all correctly set up before we > return a Node wrapper... See bug 326497 comment 11. The GetProperty is there precisely to make sure that the prototype chain is set up and delegation works. In that bug, I changed the GetProperty to a LookupProperty to avoid calling the getter, but I think the real fix is to call nsWindowSH::GlobalResolve directly from nsDOMClassInfo::PostCreate, so we'd avoid hitting the JS engine at all.
Flags: blocking1.9a1? → blocking1.9-
Assignee: jst → mrbkap
Bkap is this something we still want to look at?
Yeah, we can probably avoid doing some work by fixing this bug.
Flags: blocking1.9- → blocking1.9?
+'ing this and setting it to P3.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Johnny's being nice enough to take this.
Assignee: mrbkap → jst
I've been looking for this in performance profiles lately and it's really not anything worth changing for performance reasons any more. The most time I've seen us spend in nsDOMClassInfo::PostCreate() is < 0.2% of all the time spent in JS (during startup and other JS performance tests). IMO it's simply not worth it, at least not for 1.9. Making blocking-, holler if you disagree and have a testcase that shows where this still hurts us.
Flags: blocking1.9+ → blocking1.9-
QA Contact: ian → general
Depends on: 1442039
Investigation complete. This code is all gone. ;)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.