Closed
Bug 352394
Opened 18 years ago
Closed 18 years ago
Text control editor init expensive due to nsTextInputListener::NotifySelectionChanged
Categories
(Core :: Layout: Form Controls, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha1
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: perf)
Attachments
(1 file)
(deleted),
patch
|
MatsPalmgren_bugz
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
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...
Assignee | ||
Comment 1•18 years ago
|
||
This seems to fix a lot of the slowness for me... But can someone explain why mKnowSelectionCollapsed would need to be there?
Assignee | ||
Comment 2•18 years ago
|
||
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.
Attachment #238050 -
Flags: superreview?(roc)
Attachment #238050 -
Flags: review?(mats.palmgren)
Attachment #238050 -
Flags: superreview?(roc) → superreview+
Comment 3•18 years ago
|
||
(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•18 years ago
|
||
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.
Attachment #238050 -
Flags: review?(mats.palmgren) → review+
Comment 5•18 years ago
|
||
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.
Assignee | ||
Comment 6•18 years ago
|
||
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....
Assignee | ||
Comment 7•18 years ago
|
||
> "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!
Assignee | ||
Updated•18 years ago
|
Assignee: nobody → bzbarsky
Assignee | ||
Comment 8•18 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Comment 9•17 years ago
|
||
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•17 years ago
|
||
I filed bug 394792 on comment #5.
You need to log in
before you can comment on or make changes to this bug.
Description
•