301774 - Avoid flushing gfx buffers more frequently than refresh rate (with Cocoa widgets)

Status: VERIFIED FIXED

(Core Graveyard :: GFX: Mac, defect)

Description

[[e-mail]]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050722 Camino/0.9a2+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050722 Camino/0.9a2+

After the bug 280982 checkin my build compiled with 10.4u SDK have became very
slow at redrawing when I type in text boxes on webpages with animated gif images.

Testcase in link above ( from bug 272954 ).

The official build (10.2.8 SDK) and a 10.3.9 SDK build (only sdk difference to
my build) dont have this slowness problem.

Reproducible: Always

Steps to Reproduce:
1. Build Camino with 10.4u SDK.
2. Visit testpage above.
3. Type some text in the text box.

Actual Results:  
Browser slows down. Gif images slowly animates, while it tries to catch up what
I write. Its "beach balling" as well.

Expected Results:  
"Smooth" typing and gif animation (at least on par with the 10.2.8 and 10.3.9
SDK builds)

My .mozconfig: http://homepage.mac.com/krmathis/Trunk/mozconfig.txt

Comment 1

[Mark Mentovai]

I see this with the 10.4.0 SDK.  Backing out the patch from bug 280982 doesn't
avoid the issue.  Take a look at your own builds from a few days ago, and you'll
probably find that the testcase was always worse with a 10.4 SDK than the
official builds are.

Comment 2

[Simon Fraser [no longer active]]

So they must change something in Cocoa depending on what versions of the libs
you link with. Does Tiger have further NSView drawing optimizations?

Comment 3

[Mark Mentovai]

(In reply to comment #2)
> So they must change something in Cocoa depending on what versions of the libs
> you link with.

Wouldn't be the first time.  You most likely won't see this behavior if you
build with a 10.4 SDK and run on 10.3.  If you do, it's a problem with the SDK
itself.

> Does Tiger have further NSView drawing optimizations?

Can we take advantage of the stuff they added to allow drawing into a cache? 
http://developer.apple.com/releasenotes/Cocoa/AppKit.html, sorry, no internal
link, scroll to "NSView Drawing Redirection".

The change that caused this bug SHOULD be mentioned on that page, but a quick
search on the word "linked" doesn't turn up anything that jumps out as the
obvious candidate.

Comment 4

[L. H.]

Perhaps you are running into issues with the new coalesced updates behavior:
http://developer.apple.com/documentation/Performance/Conceptual/Drawing/Articles/FlushingContent.html
It's Tiger and Tiger-built only.

Comment 5

[[e-mail]]

(In reply to comment #1)
You are correct. Even before the 280982 checkin, my 10.4u SDK build are
noticable slower than the official build. Sorry for not giving accurate
information in my first post.

Comment 6

[Stuart Morgan]

Camino *does* have exceptionally poor typing performance with coalesced updates
enabled for it, so that's a likely explanation for what you are seeing. You
probably just need to throttle back your flushing.

Comment 7

[Simon Fraser [no longer active]]

That sounds plausible. So this is heavily dependent on bug 54153.

Comment 8

[Mark Mentovai]

Attachment: The beginning of a fix

Simon, I'm not gonna let your having taken this bug get in my way!

I'm typing this in Camino, and for the first time in forever, it doesn't
completely suck.  I'm even linked against the 10.4 libraries.

The attached patch is in no way complete.  It's a proof-of-concept type of
thing.

What I'm doing here is rate-limiting back buffer flushes.  If a flush is
requested too soon, a timer is set with a callback that flushes at a more
appropriate time.  I've done this with icky static variables because the caret
doesn't reuse rendering contexts, and because I can get away with it as long as
the caret's the only thing that calls FlushRect and the port stays the same. 
It is and it does.  That doesn't make this any less hacky.  The math is also
hackish if not altogether wrong, and you can still get caret droppings by
moving around with the cursor keys (this may not be a new problem?)  I know
it's flawed, it's just a starting point.

Where to go from here?	I'm envisioning some sort of global flush manager to
replace and rate-limit QDFlushPortBuffer calls.

Comment 9

[Mark Mentovai]

Attachment: v0.2

In the previous patch, I was flushing at a maximum of 3Hz because that provided
the only thing resembling a rational blink rate on the test page.  That problem
is purely bug 54153 and causes problems with non-Cocoa widgets too, so I won't
work around it here.  I'm now using a sane, but still hardcoded, 30Hz.	This
may change to be display-dependent based on the guidance in Apple's doc.

Drawing issues were caused by resetting the dirty region instead of adding to
it.  The caret droppings should be gone now.

I also fixed the case that would allow a pair of flushes instead of a single
flush if the timer was set but didn't fire until after a FlushRect call.

And warning cleanup too.  That's always healthy.

This patch should be suitable for testing.  From the user's standpoint, it
should be equivalent to whatever final result is produced here.  From the
developer's standpoint, it's not, but hey, it's Saturday night and I just got
home.

Comment 10

[[e-mail]]

(In reply to comment #9)
> Created an attachment (id=190290) [edit]
> v0.2

I just compiled a 10.4u SDK build and I must say this is working a LOT better
than it has for a long time. I even think its smoother than the official build...

Ok, I know its not a final solution. But you definatelly seem to be on the right
track. Great!

Comment 11

[Jasper]

I just tested this witha custom build and the attached patch make a HUGE
difference on page with a bunc of animations. This is defenitly the way to go.

Comment 12

[Simon Fraser [no longer active]]

This is certainly one approach, and if we don't get to fixing bug 54153, is a
good one. Note also that bug 287813 will make this obsolete.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We are going to land 287813 as early as possible in 1.9. So you should do what
you can for 1.8 but don't bother trying to fix it yourself for 1.9.

Comment 14

[Mark Mentovai]

Repurposing bug to match what's being done here.  Old summary: "SDK >= 10.4:
Slow redrawing when typing in text boxes on pages with animated gif images"

I'll have a clean patch later or tomorrow.  This will be nice to have for
anything that might like to flush frequently, although it seems that the caret's
the only thing that falls into that category at the moment.

Comment 15

[Mark Mentovai]

Attachment: Rate-limited QD buffer flushes, v1.0

Here it is.  This improves the snots out of typing performance with
koko-widgets, nicely implemented in a neat little globally-available flush
manager.

Comment 16

[Mark Mentovai]

Comment on attachment 190366 [details] [diff] [review]
Rate-limited QD buffer flushes, v1.0

This:
+  if (Hz == 0)
+  {
+    // Initialize the refresh rate.

is a relic and a bad idea.  Initialization won't be conditional on
uninitialized values.

Comment 17

[Mike Pinkerton (not reading bugmail)]

+// Static globally-available flush manager
+NS_EXPORT nsQDFlushManager sQDFlushManager;

rather than exporting the global, you should wrap it in a static function that
returns a ptr to the global. Helps enforce it as a singleton.

+  PRUintn                Hz;                 // Maximum number of updates per
+                                             // second

i'd say add a getter for this to enforce that it's not changable by clients.

+    nsQDFlushPort* port;
+    nsQDFlushPort* next;

don't declare these until you're ready to use them. eg

+    for (port = mPortList ; port ; port = next)

should be |for (nsQDFlushPort* port = mPortList....)|

+  nsQDFlushPort** portPtr;

initialize this to nil for safety...

+  AbsoluteTime now;
+  Nanoseconds elapsed;
+  PRInt64 timeUntilFlush;

same as above, defer declaration until you can initialize.

+  if (!mFlushTimerRunning && timeUntilFlush < 0)
+  {
+    // If past the time for the next acceptable flush, flush now.
+    ::QDFlushPortBuffer(mPort, aRegion);
+    mLastFlushTime = now;
+  }

what happens if the machine goes to sleep while this is happening? will it wake
up and flush a hell of a lot of times, or under-flush? just curious if you've
tested that aspect.

the other question is, does this really need a lock and threadsafety? who else
besides the main thread is going to be doing any drawing? Seems like overhead
that's not needed.

Comment 18

[Mark Mentovai]

Attachment: Rate-limited QD buffer flushes, v2

> rather than exporting the global, you should wrap it in a static function
> that returns a ptr to the global. Helps enforce it as a singleton.

How about this?  I've just made the methods and members static.  I'm still
keeping the "s" object around just to get the destructor to fire.  I guess even
that's not necessary since it's at quit time, but I like cleaning up after
myself.  If you think it's better without, I'll get rid of it too.

> i'd say add a getter for this to enforce that it's not changable by clients.

I stuck it in the port object as a static const.

(in CreateOrGet)
> +   nsQDFlushPort** portPtr;
> initialize this to nil for safety...

I'll initialize it to &sPortList instead and take the initialization out of the
for.

> same as above, defer declaration until you can initialize.

Yeah, pre-C99 habit.  I prefer all declarations up top, but I'll go with the
prevailing style.

> what happens if the machine goes to sleep while this is happening? will it
> wake up and flush a hell of a lot of times, or under-flush? just curious if
> you've tested that aspect.

It should flush just right.  ::UpTime stops counting when the machine is
sleeping.  Even if it didn't, with only a single instance per port, we'd see at
most one potentially (but probably not) unnecessary flush.

> the other question is, does this really need a lock and threadsafety? who
> else besides the main thread is going to be doing any drawing? Seems like
> overhead that's not needed.

I tossed that in at the last minute to make it more general.  You're right,
it's not needed here.

Other changes:
Added a TimeUntilFlush method to the port object.  This is used instead of
calculating the time directly in FlushPortBuffer.  The new method also supports
a new check in CreateOrGetPort that will repurpose existing stale port objects
once they're no longer necessary.  Now, in the most common case, the list
should remain only one entry long.  That's nice.

This will now only be part of the build if Cocoa widgets are selected, which
makes it Camino-only and npotdb.  And you know what that means.

Comment 19

[Simon Fraser [no longer active]]

Comment on attachment 190757 [details] [diff] [review]
Rate-limited QD buffer flushes, v2

Header first...

>--- /dev/null	2005-07-27 16:11:21.000000000 -0400
>+++ mozilla/gfx/src/mac/nsQDFlushManager.h	2005-07-27 16:21:14.000000000 -0400

>+#include <MacTypes.h>
>+#include <Quickdraw.h>

Shouldn't we just be using Carbon/Carbon.h these days?


>+class NS_EXPORT nsQDFlushManager
>+{
>+public:
>+  nsQDFlushManager            ();
>+  ~nsQDFlushManager           ();

This spacing is unconventional. Choose either:

    nsQDFlushManager();
    ~nsQDFlushManager();
    void			Foopy();

or

				nsQDFlushManager();
				~nsQDFlushManager();
    void			Foopy();

>+
>+  static void                 FlushPortBuffer(CGrafPtr, RgnHandle);

I like to see params named in the header too -- "inPort", "inRegion".

>+  class nsQDFlushPort : public nsITimerCallback
>+  {
>+    friend class nsQDFlushManager;
>+
>+  public:
>+                              nsQDFlushPort(CGrafPtr);
>+                              ~nsQDFlushPort();
>+
>+  protected:
>+    void                      Init(CGrafPtr);
>+    void                      Destroy();
>+    void                      FlushPortBuffer(RgnHandle);
>+    PRInt64                   TimeUntilFlush(AbsoluteTime);
>+    NS_DECL_ISUPPORTS
>+    NS_DECL_NSITIMERCALLBACK
>+
>+    // To use the display's refresh rate as the update Hz, see
>+    // http://developer.apple.com/documentation/Performance/Conceptual/Drawing/Articles/FlushingContent.html .
>+    // Here, use a hard-coded 30 Hz instead.
>+    static const PRUintn      Hz = 30;            // Maximum number of updates

The size of PRUintn depends on the compilation options; it's not a good idea
to use this here. PRUint32 will do fine. We also prefer consts to be prepended
by 'k', so kHz, or, better, kRefreshRateHz.

>+    nsQDFlushPort*            mNext;              // Next object in list
>+    CGrafPtr                  mPort;              // Associated port
>+    AbsoluteTime              mLastFlushTime;     // Last QDFlushPortBuffer call
>+    nsCOMPtr<nsITimer>        mFlushTimer;        // Timer for scheduled flush
>+    PRPackedBool              mFlushTimerRunning; // Is it?
>+  };

I'm not a big fan of nested classes; I'd prefer this be declared and
implemented
in the .cpp file, and just forward-declared here. Someone looking at how to use
this thing shouldn't have to wade through implementation classes.


>--- /dev/null	2005-07-27 16:11:21.000000000 -0400
>+++ mozilla/gfx/src/mac/nsQDFlushManager.cpp	2005-07-27 16:21:43.000000000 -0400

>+// nsQDFlushManager
>+
>+// This sets up a dummy object so the destructor will fire.
>+static nsQDFlushManager sQDFlushManager;

Eh? Is this to prevent dead-stripping or something?

>+nsQDFlushManager::~nsQDFlushManager()
>+{
>+  nsQDFlushPort* next;
>+
>+  for (nsQDFlushPort* port = sPortList ; port ; port = next)
>+  {
>+    next = port->mNext;
>+    port->Destroy();
>+    NS_RELEASE(port);

I find while loops easier to grok:

    nsQDFlushPort* port = sPortList;
    while (port)
    {
	nsQDFlushPort* next = port->mNext;
	port->Destroy();
	NS_RELEASE(port);
	port = next;
    }


>+// CreateOrGetPort(aPort)
>+//
>+// Walks through the list of port objects and returns the one corresponding to
>+// aPort if it exists.  If it doesn't exist, the port object is created, added
>+// to the list, and returned.

Say when the port object will be destroyed.

>+//
>+// protected static
>+nsQDFlushManager::nsQDFlushPort*
>+nsQDFlushManager::CreateOrGetPort(CGrafPtr aPort)
>+{
>+  AbsoluteTime now = ::UpTime();
>+  nsQDFlushPort** portPtr = &sPortList;
>+
>+  // Don't be frightened.  The pointer-pointer business isn't so confusing,
>+  // and it eases maintenance of the linked list.
>+  // First, go through the list and see if an object is already associated
>+  // with this port.
>+  for ( ; *portPtr ; portPtr = &((*portPtr)->mNext))
>+  {
>+    if (aPort == (*portPtr)->mPort)
>+    {
>+      return *portPtr;
>+    }
>+  }

Again, i think this would be clearer with a while loop (and without the weird
double deref):

    nsQDFlushPort* port = sPortList;
    while (port)
    {
	if (aPort == port->mPort)
	    return port;
	port = port->mNext;
    }

>+  // If no port object exists yet, try to find an object that's not in use.
>+  // If there is one, repurpose it.
>+  for (portPtr = &sPortList ; *portPtr ; portPtr = &((*portPtr)->mNext))
>+  {
>+    if (!(*portPtr)->mFlushTimerRunning && (*portPtr)->TimeUntilFlush(now) < 0)
>+    {
>+      // If the flush timer is not running and it's past the time during which
>+      // a flush would be postponed, the object is no longer needed.  Future
>+      // flushes for (*portPtr)->mPort would occur immediately.  Since there's
>+      // no longer any state to track, the object can be reused for another
>+      // port.  This keeps the size of the list manageable.
>+      (*portPtr)->Init(aPort);
>+      return *portPtr;
>+    }
>+  }

Another thing more clearly written with a while loop, and without the extra
deref.

>+  *portPtr = new nsQDFlushPort(aPort);
>+  NS_ADDREF(*portPtr);
>+  return *portPtr;
>+}

>+
>+// RemovePort(aPort)
>+//
>+// Walks through the list of port objects and removes the one corresponding to
>+// aPort, if it exists.
>+//
>+// Presently unused.
>+//
>+// protected static
>+void
>+nsQDFlushManager::RemovePort(CGrafPtr aPort)
>+{
>+  // Traversal is as in CreateOrGetPort.
>+  for (nsQDFlushPort** portPtr = &sPortList ; *portPtr ;
>+       portPtr = &((*portPtr)->mNext))
>+  {
>+    if (aPort == (*portPtr)->mPort)
>+    {
>+      nsQDFlushPort* next = (*portPtr)->mNext;
>+      (*portPtr)->Destroy();
>+      NS_RELEASE(*portPtr);
>+      *portPtr = next;
>+      return;
>+    }
>+  }

Isn't this broken? You have to hook up mNext of the previous port object in the
list to the next one, no?

>+nsQDFlushManager::nsQDFlushPort::nsQDFlushPort(CGrafPtr aPort)
>+: mNext(nsnull)
>+, mPort(aPort)
>+, mLastFlushTime((AbsoluteTime){0, 0})
>+, mFlushTimer(nsnull)
>+, mFlushTimerRunning(PR_FALSE)
>+{
>+}
>+
>+nsQDFlushManager::nsQDFlushPort::~nsQDFlushPort()
>+{
>+  // Everything should have been taken care of by Destroy().
>+}
>+
>+// Init(aPort)
>+//
>+// (Re)initialize object.
>+//
>+// protected
>+void
>+nsQDFlushManager::nsQDFlushPort::Init(CGrafPtr aPort)
>+{
>+  mPort = aPort;
>+  return;
>+}

No need for 'return'.

>+// FlushPortBuffer(aRegion)
>+//
>+// Flushes, dirties, and schedules, as appropriate.  Public access is from
>+// nsQDFlushManager::FlushPortBuffer(CGrafPtr aPort, RgnHandle aRegion).
>+//
>+// protected
>+void
>+nsQDFlushManager::nsQDFlushPort::FlushPortBuffer(RgnHandle aRegion)
>+{
>+  AbsoluteTime now = ::UpTime();
>+  PRInt64 timeUntilFlush = TimeUntilFlush(now);
>+
>+  if (!mFlushTimerRunning && timeUntilFlush < 0)
>+  {
>+    // If past the time for the next acceptable flush, flush now.
>+    ::QDFlushPortBuffer(mPort, aRegion);
>+    mLastFlushTime = now;
>+  }
>+  else
>+  {
>+    // If it's not time for the next flush yet, or if the timer is running
>+    // indicating that an update is pending, just mark the dirty region.
>+    ::QDAddRegionToDirtyRegion(mPort, aRegion);

How does QDAddRegionToDirtyRegion() interplay with QDFlushPortBuffer()?
Have you checked to see that we're just flushging the union of the flush
regions, and not the entire port?

>+
>+    if (!mFlushTimerRunning)
>+    {
>+      // No flush scheduled?  No problem.
>+      if (!mFlushTimer)
>+      {
>+        // No timer object?  No problem.
>+        nsresult err;
>+        mFlushTimer = do_CreateInstance("@mozilla.org/timer;1", &err);
>+        NS_ASSERTION(NS_SUCCEEDED(err), "Could not instantiate flush timer.");
>+      }
>+      if (mFlushTimer)
>+      {
>+        // Start the clock, with the timer firing at the already-calculated
>+        // time until the next flush.  Nanoseconds (1E-9) were used above,
>+        // but nsITimer is big on milliseconds (1E-3), so divide by 1E6.
>+        // Any time that was consumed between the ::UpTime call and now
>+        // will be lost.  That's not so bad in the usual case, it's a tiny
>+        // bit less not so bad if a timer object didn't exist yet and was
>+        // created.  It's better to update slightly less frequently than
>+        // the target than slightly more frequently.
>+        mFlushTimer->InitWithCallback(this, (PRUint32)(timeUntilFlush/1E6),
>+                                      nsITimer::TYPE_ONE_SHOT);
>+        mFlushTimerRunning = PR_TRUE;
>+      }
>+    }
>+  }
>+  return;

Nuke return.


>Index: mozilla/gfx/src/mac/nsRenderingContextMac.cpp
>===================================================================

>-    RgnHandle rgn = ::NewRgn();
>+    RgnHandle rgn = sNativeRegionPool.GetNewRegion();

StRegionFromPool ?

Comment 20

[Mark Mentovai]

(In reply to comment #19)
> The size of PRUintn depends on the compilation options; it's not a good idea
> to use this here. PRUint32 will do fine. We also prefer consts to be prepended
> by 'k', so kHz, or, better, kRefreshRateHz.

Yeah, I didn't want to use kHz for obvious reasons.  I'll be verbose to work the
k in.

> >+static nsQDFlushManager sQDFlushManager;
> 
> Eh? Is this to prevent dead-stripping or something?

It makes sure the destructor fires while avoiding dead-stripping problems.  Like
I said, the destructor's not really strictly necessary if everything's static
anyway.

> Again, i think this would be clearer with a while loop (and without the weird
> double deref):

(in CreateOrGetPort:)
I can get rid of the pointer-to-pointer in the first loop for sure.  It's
vestigal.  I'm more attached to it in the second pass, though, so that portPtr
points to either the last mNext or, if none, sPortList.  That lets me just stuff
the new object into *portPtr.  I find the alternative to pointer fun ugly and
unnecessary:

  if (sPortList == nsnull)
    sPortList = the_new_object;
  else
    port->mNext = the_new_object;

> Isn't this broken? You have to hook up mNext of the previous port object in 
> the list to the next one, no?

(in RemovePort:)
As above, *portPtr is either sPortList if removing the first or mNext of the
previous if removing anything else.  In this case, the alternative is even uglier.

> How does QDAddRegionToDirtyRegion() interplay with QDFlushPortBuffer()?
> Have you checked to see that we're just flushging the union of the flush
> regions, and not the entire port?

The documented behavior is that QDFlushPortBuffer() flushes only the dirty
region which I'm setting with QDAddRegionToDirtyRegion() plus whatever region is
passed directly to QDFlushPortBuffer().  If that directly-passed region is NULL,
only the dirty region is flushed.

> >-    RgnHandle rgn = ::NewRgn();
> >+    RgnHandle rgn = sNativeRegionPool.GetNewRegion();
> 
> StRegionFromPool ?

(in nsRenderingContextMac.cpp:)
OK, and I'll clean up other occurrences in that file too.

Comment 21

[Mark Mentovai]

Is there a preferred style to use in preference to static ctors and dtors?

I'm thinking of taking all of the static methods and members in nsQDFlushManager
back to non-static and providing a static nsQDFlushManager *sQDFlushManager in
the class, along with a static sFlushPortBuffer that instantiates
sQDFlushManager if necessary and calls sQDFlushManager->FlushPortBuffer().  The
dummy object to get the dtor firing is stupid, and I'd rather not leak.

Comment 22

[Mark Mentovai]

Attachment: Rate-limited QD buffer flushes, v3

Style and readability changes made.  Things that were brought up that I haven't
already commented on are integrated as-is.  I'm still doing pointer-pointer
where it's advantageous to do so, but it should be much more readable now
because I've added explicit shadow derefs and comments that should do a better
job of explaining what portPtr is about.  I've modified the manager class to be
non-static while carrying a static member and associated static flush method as
proposed in comment 21.  The static method is responsible for instantiation,
and arranges for destruction at shutdown via a shutdown observer.  This is all
explained in the comments.

Comment 23

[Mark Mentovai]

Needs #include "nsCRT.h" (but only when building debug?!)

Comment 24

[[e-mail]]

Mark, you might want to take a look at bug 302826
Starting yesterday it caused a failed build on my 10.4.2, GCC 4.0.0, 10.4u SDK
setup.

After I backed out the patch from my source it compiled successfully!

Comment 25

[Mark Mentovai]

Kai, I can't reproduce what was bug 302826.  None of the ld output you pasted
there is an error, there are only warnings.  If you're still experiencing
trouble, reopen that bug and give me a more complete dump of the final g++ -o
mozilla-bin that fails.

Comment 26

[Mark Mentovai]

Sorry, I missed the error there because the bug summary was misleading.  Still,
I can't reproduce.  I changed some things between v2 and v3 of the patch that
might cause this error if you don't cleanly and properly back out v2 before
applying v3.  Try pulling a fresh source tree in at least mozilla/gfx/src/mac
and reapplying the most recent version of the patch.

Comment 27

[Mark Mentovai]

Attachment: Rate-limited QD buffer flushes, v3, includes Makefile patch

The problem Kai was seeing was due to my having not included the Makefile diff
in 301774.5.  This version corrects that omission, but is otherwise identical.

Comment 28

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 191418 [details] [diff] [review]
Rate-limited QD buffer flushes, v3, includes Makefile patch

r=pink

Comment 29

[[e-mail]]

(In reply to comment #27)

Thanks Mark!
After I used the 301774.6.patch Camino built successfully again.

Comment 30

[Mark Mentovai]

Checked in.