Should batch reflows and turn off painting around UpdateCommands

RESOLVED WONTFIX

Status

()

Core
XUL
P3
normal
RESOLVED WONTFIX
18 years ago
15 years ago

People

(Reporter: Simon Fraser, Assigned: Simon Fraser)

Tracking

({helpwanted, perf})

Trunk
Future
helpwanted, perf
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Assignee)

Description

18 years ago
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.
(Assignee)

Comment 1

18 years ago
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);
     }
 
Status: NEW → ASSIGNED
(Assignee)

Comment 2

18 years ago
perf
Keywords: perf
(Assignee)

Comment 3

18 years ago
waterson: can I get this in by you as a mozilla.org checkin? It makes a big 
difference for the composer window.
Target Milestone: --- → M18

Comment 4

18 years ago
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

18 years ago
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.
(Assignee)

Comment 6

18 years ago
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

18 years ago
moving to m19
Target Milestone: M18 → M19

Comment 8

18 years ago
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

18 years ago
moving to future per review with bij and beppe
Keywords: helpwanted
Target Milestone: M19 → Future

Comment 10

17 years ago
Changing platform
Hardware: All → Macintosh

Updated

17 years ago
OS: Mac System 8.5 → All
Hardware: Macintosh → All

Comment 11

16 years ago
removing myself from the cc list

Comment 12

15 years ago
What's the approach on that now? Seems to be really reasonable.
(Assignee)

Comment 13

15 years ago
This bug is pretty stale now. I'm not sure if it's still a problem, or if the
patch is worthwhile.
Assignee: sfraser → jaggernaut
Status: ASSIGNED → NEW

Comment 14

15 years ago
-> sfraser

Simon, what do you want to do with this?
Assignee: jaggernaut → sfraser
(Assignee)

Comment 15

15 years ago
Oldy crufy.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.