509889 - nsEventStateManager::GetContentState needs fast path to nsFocusManager::GetFocusedElement

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

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

I just did a profile of new-browser-window performance in Firefox, and noticed that we're spending ~1% of the time (in whatever callgrind measures it in, which isn't real time, but it's something) doing nsEventStateManager::GetContentState's call to nsFocusManager::GetFocusedElement, which involves doing a QueryInterface from nsIContent to nsIDOMElement, returning the object, and then QIing the other way, and the ensuing reference counting and nsCOMPtr machinery.

(GetContentState is called a huge number of times because we call it during selector matching... and up-front on the assumption that it will be cheap.)

nsFocusManager ought to have a de-COM-taminated getter that returns a raw nsIContent pointer (not AddRef-ed).

Neil, do you have a preference between:

 (1) putting a second GetFocusedElement method on nsFocusManager and make nsFocusManager::GetFocusManager return an nsFocusManager instead of nsIFocusManager, or

 (2) adding a static nsFocusManager::GetFocusedElement method that specifically called an nsFocusManager method on sInstance (and only change the type of sInstance and not GetFocusManager)?

Comment 1

[Neil Deakin]

Number 1 sounds better.

Comment 2

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

Attachment: patch

Comment 3

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

http://hg.mozilla.org/mozilla-central/rev/f555ee21c59a

Comment 4

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

Comment on attachment 394187 [details] [diff] [review]
patch

Very simple de-COM-tamination performance win, fixing a regression from the focus manager landing.

Comment 5

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

catlee's performance monitoring scripts reported around a 1% Tdhtml win when this landed, but didn't report any Txul changes.  I landed it with some other large patches, though, but I think it's likely this one:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e0cc032cb852&tochange=e6034ded61fd

Comment 6

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c64779a5e8ca

Comment 7

[Boris Zbarsky [:bzbarsky]]

This reduced Tdhtml 1% on 1.9.2 as well (and landed there alone, so gets the credit all to itself).