investigate whether JS_GetProperty in nsDOMClassInfo::PostCreate is needed

NEW
Assigned to

Status

()

Core
DOM
P3
normal
13 years ago
8 years ago

People

(Reporter: Brian Ryner (not reading), Assigned: jst)

Tracking

(Blocks: 1 bug, {helpwanted, perf})

Trunk
helpwanted, perf
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

(Reporter)

Description

13 years ago
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.

Updated

12 years ago
Blocks: 335763

Updated

11 years ago
Flags: blocking1.9a1? → blocking1.9-

Updated

11 years ago
Assignee: jst → mrbkap

Comment 10

10 years ago
Bkap is this something we still want to look at?
Yeah, we can probably avoid doing some work by fixing this bug.

Updated

10 years ago
Flags: blocking1.9- → blocking1.9?
+'ing this and setting it to P3.  
Flags: blocking1.9? → blocking1.9+

Updated

10 years ago
Priority: -- → P3
Johnny's being nice enough to take this.
Assignee: mrbkap → jst
(Assignee)

Comment 14

10 years ago
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
You need to log in before you can comment on or make changes to this bug.