44162 - Should batch reflows and turn off painting around UpdateCommands

Status: RESOLVED WONTFIX

(Core :: XUL, defect, P3)

Description

[Simon Fraser [no longer active]]

Since code called by UpdateCommands can cause lots of attribute setting, which 
can cause reflows and painting, we should batch reflows and view manager updates 
around this call.

Comment 1

[Simon Fraser [no longer active]]

Here's a diff to fix this. This makes quite an impact on editor window loading.

Index: nsGlobalWindow.cpp
===================================================================
RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v
retrieving revision 1.307
diff -u -2 -r1.307 nsGlobalWindow.cpp
--- nsGlobalWindow.cpp	2000/06/27 22:13:09	1.307
+++ nsGlobalWindow.cpp	2000/06/29 00:35:00
@@ -1829,5 +1829,25 @@
       nsCOMPtr<nsIDOMXULCommandDispatcher> xulCommandDispatcher;
       xulDoc->GetCommandDispatcher(getter_AddRefs(xulCommandDispatcher));
+      
+      // Batch reflows
+      PRBool  wasBatching = PR_FALSE;
+      nsCOMPtr<nsIPresShell> presShell;
+      nsCOMPtr<nsIViewManager> viewManager;
+      
+      mDocShell->GetPresShell(getter_AddRefs(presShell));
+      if (presShell) {
+        presShell->GetViewManager(getter_AddRefs(viewManager));
+        if (viewManager)
+          viewManager->BeginUpdateViewBatch();
+        presShell->GetReflowBatchingStatus(&wasBatching);
+        presShell->BeginReflowBatching();
+      }
+      
       xulCommandDispatcher->UpdateCommands(anAction);
+      
+      if (presShell)
+        presShell->EndReflowBatching(wasBatching);
+      if (viewManager)
+        viewManager->EndUpdateViewBatch(0);
     }
 

Comment 2

[Simon Fraser [no longer active]]

perf

Comment 3

[Simon Fraser [no longer active]]

waterson: can I get this in by you as a mozilla.org checkin? It makes a big 
difference for the composer window.

Comment 4

[Chris Waterson]

Well, since you're not a "non-Netscape contributor", no, I'm not going pretend
you are. But I'm not your boss, nor am I the police. If you think it makes a big
enough difference and are willing to risk a "talking to" from the PDT, then
check it in. The patch certainly seems extremely reasonable.

Comment 5

[David Hyatt]

Wouldn't it be better to do this inside the command dispatcher's UpdateCommands 
method?  Then everyone benefits, not just people who use the DOM window method.

Comment 6

[Simon Fraser [no longer active]]

If I did this in nsXULCommandDispatcher::UpdateCommands, I'd have to do it inside 
the Updater loop (where I have a presShell). That means that I might be turning 
batching on and off multiple times per presShell. I guess I could run through the 
Updaters first, and turn on batching once per unique presShell, then off again at 
the end. But that means more code.

Comment 7

[rubydoo123]

moving to m19

Comment 8

[rubydoo123]

please do not nominate for nsbeta3 -- this will be addressed after crashes, 
regressions, correctness and polish bugs have been resolved, all Composer perf 
bugs have been set to m19/m20

Comment 9

[rubydoo123]

moving to future per review with bij and beppe

Comment 10

[basic]

Changing platform

Comment 11

[rubydoo123]

removing myself from the cc list

Comment 12

[Markus Hübner]

What's the approach on that now? Seems to be really reasonable.

Comment 13

[Simon Fraser [no longer active]]

This bug is pretty stale now. I'm not sure if it's still a problem, or if the
patch is worthwhile.

Comment 14

[jag (Peter Annema)]

-> sfraser

Simon, what do you want to do with this?

Comment 15

[Simon Fraser [no longer active]]

Oldy crufy.