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

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.

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

(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.

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.

(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.

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.

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

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.

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.

(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!

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.

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.

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.

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.

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 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.

+// 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.

> 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 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 ?

(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.

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.

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.

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

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!

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.

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.

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 on attachment 191418 [details] [diff] [review] Rate-limited QD buffer flushes, v3, includes Makefile patch r=pink

(In reply to comment #27) Thanks Mark! After I used the 301774.6.patch Camino built successfully again.

Checked in.