STR: 1) open up the dhtml bubble mark demo - http://bubblemark.com/dhtml.htm 2) open up task manager and highlight firefox.exe 3) hover over the firefox taskbar icon so that you get a preview rendering of the bubble mark tab. Typical behavior is one of two cases: 1) cpu spikes 2) memory use climbs upward of around 150 megs, then fall back down, then climbs again, over and over. or 1) cpu spikes 2) memory use climbs upwards around 500 megs, previews die, and sometimes the browser locks or UI/window frame rendering breaks. You can make things worse and get to the second case by opening up multiple tabs to the bubble mark dhtml page.

This really helps.

Ok, added those changes we spoke about on irc.

Comment on attachment 439981 [details] [diff] [review] updated patch >diff --git a/content/canvas/public/nsICanvasRenderingContextInternal.h b/content/canvas/public/nsICanvasRenderingContextInternal.h >--- a/content/canvas/public/nsICanvasRenderingContextInternal.h >+++ b/content/canvas/public/nsICanvasRenderingContextInternal.h >@@ -82,14 +82,17 @@ public: > // return the surface. Otherwise returns an error. > NS_IMETHOD GetThebesSurface(gfxASurface **surface) = 0; > > // If this context is opaque, the backing store of the canvas should > // be created as opaque; all compositing operators should assume the > // dst alpha is always 1.0. If this is never called, the context > // defaults to false (not opaque). > NS_IMETHOD SetIsOpaque(PRBool isOpaque) = 0; >+ >+ // Destroy thebes/image stuff, in preparation for possibly recreating. >+ NS_IMETHOD Destroy() = 0; > }; I don't think we need to COMify it with the nsresult return code but I'll defer to vlad here. >+ // create the canvas rendering context >+ nsresult rv; >+ mCtxI = do_CreateInstance("@mozilla.org/content/canvas-rendering-context;1?id=2d", &rv); >+ if (NS_FAILED(rv)) { >+ NS_WARNING("Could not create nsICanvasRenderingContextInternal for tab previews!"); >+ } >+ It occurs to me that we really only need one of these since tab previews are always run on the same thread. Perhaps we can lazily create a static one? We should still ref count it to avoid having to deal with the xpcom-shutdown event. >+ // Purge the memory for gfxWindowsSurface >+ mCtxI->Destroy(); Purge isn't really the right word - we want to tell mCtx to release its grip on the surface. Also since this is an API change, requesting SR from vlad.

> Also since this is an API change, requesting SR from vlad. I need to bump the uid too. Oops!

Comment on attachment 439981 [details] [diff] [review] updated patch >diff --git a/content/canvas/public/nsICanvasRenderingContextInternal.h b/content/canvas/public/nsICanvasRenderingContextInternal.h >--- a/content/canvas/public/nsICanvasRenderingContextInternal.h >+++ b/content/canvas/public/nsICanvasRenderingContextInternal.h >@@ -82,14 +82,17 @@ public: > // return the surface. Otherwise returns an error. > NS_IMETHOD GetThebesSurface(gfxASurface **surface) = 0; > > // If this context is opaque, the backing store of the canvas should > // be created as opaque; all compositing operators should assume the > // dst alpha is always 1.0. If this is never called, the context > // defaults to false (not opaque). > NS_IMETHOD SetIsOpaque(PRBool isOpaque) = 0; >+ >+ // Destroy thebes/image stuff, in preparation for possibly recreating. >+ NS_IMETHOD Destroy() = 0; Hmm. How about instead of this, we call this Reset()? And have the comment say something like 'Invalidate this context and release any held resources, in preperation for possibly reinitializing with SetDimensions/InitializeWithSurface'. Destroy sounds a lot more final -- in other usages of Destroy in the tree, after Destroy is called, that object is no longer valid to be used at all. Destroy isn't the greatest name inside 2d.cpp either, but that was a more private usage. If you change this to Reset() it's fine to change the 2d function to Reset() as well. >diff --git a/widget/src/windows/TaskbarPreview.cpp b/widget/src/windows/TaskbarPreview.cpp >--- a/widget/src/windows/TaskbarPreview.cpp >+++ b/widget/src/windows/TaskbarPreview.cpp >@@ -52,62 +52,43 @@ > #include <nsServiceManagerUtils.h> > > #include "nsUXThemeData.h" > #include "nsWindow.h" > #include "nsAppShell.h" > #include "TaskbarPreviewButton.h" > > #include <nsIBaseWindow.h> >-#include <nsICanvasRenderingContextInternal.h> > #include <nsIDOMCanvasRenderingContext2D.h> > #include <imgIContainer.h> > #include <nsIDocShell.h> > > // Defined in dwmapi in a header that needs a higher numbered _WINNT #define > #define DWM_SIT_DISPLAYFRAME 0x1 > > namespace mozilla { > namespace widget { > >-namespace { >-/* Helper method to create a canvas rendering context backed by the given surface >- * >- * @param shell The docShell used by the canvas context for text settings and other >- * misc things. >- * @param surface The gfxSurface backing the context >- * @param width The width of the given surface >- * @param height The height of the given surface >- * @param aCtx Out-param - a canvas context backed by the given surface >- */ >-nsresult >-CreateRenderingContext(nsIDocShell *shell, gfxASurface *surface, PRUint32 width, PRUint32 height, nsICanvasRenderingContextInternal **aCtx) { >- nsresult rv; >- nsCOMPtr<nsICanvasRenderingContextInternal> ctx(do_CreateInstance( >- "@mozilla.org/content/canvas-rendering-context;1?id=2d", &rv)); >- NS_ENSURE_SUCCESS(rv, rv); >- rv = ctx->InitializeWithSurface(shell, surface, width, height); >- NS_ENSURE_SUCCESS(rv, rv); >- >- NS_ADDREF(*aCtx = ctx); >- return NS_OK; >-} >- >-} >- > TaskbarPreview::TaskbarPreview(ITaskbarList4 *aTaskbar, nsITaskbarPreviewController *aController, HWND aHWND, nsIDocShell *aShell) > : mTaskbar(aTaskbar), > mController(aController), > mWnd(aHWND), > mVisible(PR_FALSE), > mDocShell(do_GetWeakReference(aShell)) > { > // TaskbarPreview may outlive the WinTaskbar that created it > ::CoInitialize(NULL); > >+ // create the canvas rendering context >+ nsresult rv; >+ mCtxI = do_CreateInstance("@mozilla.org/content/canvas-rendering-context;1?id=2d", &rv); >+ if (NS_FAILED(rv)) { >+ NS_WARNING("Could not create nsICanvasRenderingContextInternal for tab previews!"); >+ } >+ > WindowHook &hook = GetWindowHook(); > hook.AddMonitor(WM_DESTROY, MainWindowHook, this); > } > > TaskbarPreview::~TaskbarPreview() { > // Avoid dangling pointer > if (sActivePreview == this) > sActivePreview = nsnull; >@@ -346,23 +327,24 @@ TaskbarPreview::UpdateTooltip() { > > void > TaskbarPreview::DrawBitmap(PRUint32 width, PRUint32 height, PRBool isPreview) { > nsresult rv; > nsRefPtr<gfxWindowsSurface> surface = new gfxWindowsSurface(gfxIntSize(width, height), gfxASurface::ImageFormatARGB32); > > nsCOMPtr<nsIDocShell> shell = do_QueryReferent(mDocShell); > >- if (!shell) >+ if (!shell || !mCtxI) > return; > >- nsCOMPtr<nsICanvasRenderingContextInternal> ctxI; >- rv = CreateRenderingContext(shell, surface, width, height, getter_AddRefs(ctxI)); >+ rv = mCtxI->InitializeWithSurface(shell, surface, width, height); >+ if (NS_FAILED(rv)) >+ return; See comment below. I'd keep CreateRenderingContext or at least move the creation code here, on first use. > >- nsCOMPtr<nsIDOMCanvasRenderingContext2D> ctx = do_QueryInterface(ctxI); >+ nsCOMPtr<nsIDOMCanvasRenderingContext2D> ctx = do_QueryInterface(mCtxI); > > PRBool drawFrame = PR_FALSE; > if (NS_SUCCEEDED(rv) && ctx) { > if (isPreview) > rv = mController->DrawPreview(ctx, &drawFrame); > else > rv = mController->DrawThumbnail(ctx, width, height, &drawFrame); > >@@ -375,16 +357,19 @@ TaskbarPreview::DrawBitmap(PRUint32 widt > HBITMAP hBitmap = (HBITMAP)GetCurrentObject(hDC, OBJ_BITMAP); > > DWORD flags = drawFrame ? DWM_SIT_DISPLAYFRAME : 0; > POINT pptClient = { 0, 0 }; > if (isPreview) > nsUXThemeData::dwmSetIconicLivePreviewBitmapPtr(PreviewWindow(), hBitmap, &pptClient, flags); > else > nsUXThemeData::dwmSetIconicThumbnailPtr(PreviewWindow(), hBitmap, flags); >+ >+ // Purge the memory for gfxWindowsSurface >+ mCtxI->Destroy(); Well, if Destroy() is a real NS_IMETHOD, then you need to check for error here... I'd suggest checking for error, and on error, just nulling out mCtxI. Above, check if it's null, and if it is then recreate it.

Comment on attachment 440303 [details] [diff] [review] updated patch v.2 Addressed various comments. (In reply to comment #3) > >+ // create the canvas rendering context > >+ nsresult rv; > >+ mCtxI = do_CreateInstance("@mozilla.org/content/canvas-rendering-context;1?id=2d", &rv); > >+ if (NS_FAILED(rv)) { > >+ NS_WARNING("Could not create nsICanvasRenderingContextInternal for tab previews!"); > >+ } > >+ > > It occurs to me that we really only need one of these since tab previews are > always run on the same thread. Perhaps we can lazily create a static one? We > should still ref count it to avoid having to deal with the xpcom-shutdown > event. I changed things up so that we lazily create it, but I didn't make it static. Seems safer to tear it down with the class, and we only have one instance of this in the browser afaict.

Comment on attachment 440303 [details] [diff] [review] updated patch v.2 sorry for the spam, rob pointed out the obvious, making the ctx static will save some more. another rev coming up.

Comment on attachment 440773 [details] [diff] [review] patch ok, I split this up into two functions, and kept the internal ctx isolated.

Comment on attachment 440773 [details] [diff] [review] patch > NS_IMETHODIMP >-nsCanvasRenderingContext2D::InitializeWithSurface(nsIDocShell *docShell, gfxASurface *surface, PRInt32 width, PRInt32 height) { >- Destroy(); >+nsCanvasRenderingContext2D::InitializeWithSurface(nsIDocShell *docShell, gfxASurface *surface, PRInt32 width, PRInt32 height) >+{ >+ Reset(); The bracing style changed - was this intentional? Also it occurs to me that we never release the context under successful conditions - when would be a good time to do this, xpcom-shutdown?

sorry it took so long to get back to this! (In reply to comment #12) > (From update of attachment 440773 [details] [diff] [review]) > > > NS_IMETHODIMP > >-nsCanvasRenderingContext2D::InitializeWithSurface(nsIDocShell *docShell, gfxASurface *surface, PRInt32 width, PRInt32 height) { > >- Destroy(); > >+nsCanvasRenderingContext2D::InitializeWithSurface(nsIDocShell *docShell, gfxASurface *surface, PRInt32 width, PRInt32 height) > >+{ > >+ Reset(); > > The bracing style changed - was this intentional? Yes, it didn't match the rest of the file. > Also it occurs to me that we never release the context under successful > conditions - when would be a good time to do this, xpcom-shutdown? Added observer support for this.

Comment on attachment 442112 [details] [diff] [review] patch v.2 Let's nix the one observer per TaskbarPreview (I also want to point out there's the shiny new mozilla::services::GetObserverService()). Alternatively, we could take the nsWindow approach to shared resource usage which is to count the number of TaskbarPreview instances and when the count goes to 0, release the shared context. Since TaskbarPreviews are XPCOM objects, they will die some time before xpcom-shutdown comes along and perhaps much sooner if for some reason the application uses taskbar previews only temporarily (Firefox could do this someday if we were so inclined). We can also avoid the nsIObserver hassles this way.

(In reply to comment #14) > (From update of attachment 442112 [details] [diff] [review]) > Let's nix the one observer per TaskbarPreview (I also want to point out there's > the shiny new mozilla::services::GetObserverService()). Alternatively, we could > take the nsWindow approach to shared resource usage which is to count the > number of TaskbarPreview instances and when the count goes to 0, release the > shared context. Since TaskbarPreviews are XPCOM objects, they will die some > time before xpcom-shutdown comes along and perhaps much sooner if for some > reason the application uses taskbar previews only temporarily (Firefox could do > this someday if we were so inclined). We can also avoid the nsIObserver hassles > this way. ref counting it is, patch coming up.

Comment on attachment 444031 [details] [diff] [review] patch v.3 passes try fine.

(In reply to comment #17) > (From update of attachment 444031 [details] [diff] [review]) > passes try fine. although I guess without win7 coverage, that's not worth much!

Comment on attachment 444031 [details] [diff] [review] patch v.3 > NS_IMETHODIMP >-nsCanvasRenderingContext2D::InitializeWithSurface(nsIDocShell *docShell, gfxASurface *surface, PRInt32 width, PRInt32 height) { >- Destroy(); >+nsCanvasRenderingContext2D::InitializeWithSurface(nsIDocShell *docShell, gfxASurface *surface, PRInt32 width, PRInt32 height) >+{ >+ Reset(); nit: bracing style change. > namespace { >-/* Helper method to create a canvas rendering context backed by the given surface >+ >+/* Helper method to lazily create a canvas rendering context and associate a given >+ * surface with it. > * > * @param shell The docShell used by the canvas context for text settings and other > * misc things. > * @param surface The gfxSurface backing the context > * @param width The width of the given surface > * @param height The height of the given surface >- * @param aCtx Out-param - a canvas context backed by the given surface > */ >+static nsCOMPtr<nsIDOMCanvasRenderingContext2D> gCtx; >+ static is not needed since we're in an anonymous namespace here. Also, the comment refers to a method but immediately precedes the member. > nsresult >-CreateRenderingContext(nsIDocShell *shell, gfxASurface *surface, PRUint32 width, PRUint32 height, nsICanvasRenderingContextInternal **aCtx) { >+CreateRenderingContext(nsIDocShell *shell, gfxASurface *surface, >+ PRUint32 width, PRUint32 height) { Let's rename this to GetRenderingContext since there's only ever one. >+ >+ // Used in tracking the number of previews. Used in freeing >+ // the static 2d rendering context on shutdown. >+ static PRUint32 mInstCount; Statics have the s prefix. Since this is private, we could also stash it in the anonymous namespace. Because this does not require additional headers and it concisely relates to the instance count of the class, I think it's ok to leave it in the class though it's your call.

This had bitrot so I updated it and addressed my nits. I also removed the global nsCOMPtr since that hurts startup perf. Jim, quick review? sr from vlad carries over still I believe. Also, this makes AeroPeek in even debug builds acceptably fast. I don't know why canvas interface creation is so expensive!

Comment on attachment 466691 [details] [diff] [review] patch v.4 Thanks for finishing this up!

Comment on attachment 466691 [details] [diff] [review] patch v.4 Requesting approval for landing on m-c. Adds a nice speedup and cuts down on perceived memory usage (especially for D2D).

Same contents as before but with the proper user and commit message.