107265 - 2.8% of window close time in JS_ClearScope

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

2.8% of window close time (23 hits in the profile cited on bug 104127) is spent
in JS_ClearScope, at the stack:

js_FlushPropertyCacheByProp
js_DestroyScopeProperty
js_DropScopeProperty
js_free_symbol
JS_HashTableDestroy 
js_hash_scope_clear 
js_Clear
JS_ClearScope
GlobalWindowImpl::SetNewDocument(nsIDOMDocument*, int)
DocumentViewerImpl::Close()
nsDocShell::Destroy()

(Well, ok, this is the stack for 22 of the 23 hits in JS_ClearScope -- the other
hit I saw was locking within JS_Free from within js_hash_scope_clear.)

Comment 1

[Brendan Eich [:brendan]]

I'll make this better now.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 3

[Brendan Eich [:brendan]]

I want to nail this before double hashing, which I need to get back to stat. 
How about fast r= and sr= if this makes dbaron happy?

/be

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Re-profiling showed much fewer hits in one profile (and mostly in free) and no
hits at all in another profile, so this seems to have helped.

Comment 5

[Mike Shaver (:shaver emeritus)]

Comment on attachment 55482 [details] [diff] [review]
proposed fix

r=shaver.

Comment 6

[John Bandhauer]

Comment on attachment 55482 [details] [diff] [review]
proposed fix

sr=jband
No nesting can prematurely clear cx->clearingScope in a way that would matter?

Comment 7

[Brendan Eich [:brendan]]

jband: control flows into clear, then to js_free_symbol, to
js_DropScopeProperty, which calls js_DestroyScopeProperty on last deref; the
only other calls are to js_FreeSlot, js_FlushPropertyCacheByProp, and JS_free. 
No nesting possible on a given cx.

/be

Comment 8

[Mike Shaver (:shaver emeritus)]

Hey, you know, someone later might change this code and not realize that the
lack of nesting is really important.  JS_ASSERT(cx->clearingScope) before you
set it to false, as a safety net?

Comment 9

[Brendan Eich [:brendan]]

shaver, I'm about to decimate this code for double hashing.  See bug 62164.  But
ok, I'll add an assert for the short run ("someone later" -- as if!).

/be

Comment 10

[Mike Shaver (:shaver emeritus)]

Yeah, yeah.  Indulge me.

Comment 11

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 12

[Phil Schwartau]

Marking Verified based on dbaron's findings at 2001-10-28 16:52 above -