352394 - Text control editor init expensive due to nsTextInputListener::NotifySelectionChanged

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Boris Zbarsky [:bzbarsky]]

On the large testcase in bug 352367 we have:

Total hit count: 1281040

172818 nsTextInputListener::NotifySelectionChanged

That's about 15% of total page load time!

The "initial" caller here is nsTypedSelection::EndBatchChanges, called from nsTextControlFrame::SetValue during editor init.

If we actually look at where time is spent under NotifySelectionChanged, it looks like calling UpdateCommands on the global window notifies the XUL command dispatcher (nsXULCommandDispatcher::UpdateCommands), which fires a DOM event, which runs a bunch of JS, etc.  This is for every single text control in the page.

Fixing bug (so doing lazy editor init) might help here, but does NotifySelectionChanged need to be this expensice?  Esp. when the selection did NOT change?

I'd really like to hear from someone familiar with our command stuff here...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Silly patch

This seems to fix a lot of the slowness for me...  But can someone explain why mKnowSelectionCollapsed would need to be there?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 238050 [details] [diff] [review]
Silly patch

I checked some more, and I can't see a way for this class to have a selection when created... so I think this patch is in fact good.

Comment 3

[Simon Fraser [no longer active]]

(In reply to comment #2)
> (From update of attachment 238050 [details] [diff] [review] [edit])
> I checked some more, and I can't see a way for this class to have a selection
> when created... so I think this patch is in fact good.

"to have a selection", or "to have a non-collapsed selection"?

(In reply to comment #0)

> If we actually look at where time is spent under NotifySelectionChanged, it
> looks like calling UpdateCommands on the global window notifies the XUL command
> dispatcher (nsXULCommandDispatcher::UpdateCommands), which fires a DOM event,
> which runs a bunch of JS, etc.  This is for every single text control in the
> page.

Ick. There's no reason commands have to be updated that many times. The UpdateCommands should be coalesced with an event or something.

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 238050 [details] [diff] [review]
Silly patch

Looks good to me. I tested this quite a bit and I couldn't find any
new problems (found an existing one though: bug 352446).
r=mats

I agree with Simon regarding coalescing the notifications, but that
should probably be done in nsXULCommandDispatcher so that we can
control the order and only coalesce a sequence of the same type.

Comment 5

[[e-mail]]

Actually the command updater events only care about the focused element/window, so a) focus updates should of course always be sent b) clipboard updates should always be sent, in case the focused element can or can no longer paste and c) select (not to be confused with DOM select events, which always need to be sent) and undo events only need to be sent if the element has focus.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So could we spin off a separate bug about the command updater?  And possibly another one on Places blindly updating the world no matter what command is updated?  Someone who understands commands better than I should file those bugs, imo....

Comment 7

[Boris Zbarsky [:bzbarsky]]

> "to have a selection", or "to have a non-collapsed selection"?

Well.  There's no text in the editor at the time.  So probably even the former...  Once we do set text on the editor, the selection is guaranteed to be collapsed.  So definitely the latter.

Thanks for the reviews, guys!

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed.  Neil, Simon, Mats please do file the other bugs in question, or catch me on IRC if you have questions so we can get them filed?  Given the past issues we've had with the performance of this sort of stuff, it'd be good to do what we can to improve it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I filed bug 394694 on the command dispatcher and bug 394695 on Places.  Really would have been nice if someone who knows what they're talking about had done it, though.  :(

Comment 10

[[e-mail]]

I filed bug 394792 on comment #5.