1130848 - performance penalty from creating extra widget / compositor due to initial about:blank load in window

Status: RESOLVED INVALID

(Core :: Layout, defect)

Description

[Steve Williams]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150202183609

Steps to reproduce:

put a breakpoint on nsWindow::~nsWindow() during mozilla initialize & noticed a window with compositor backend was built, then torn down and rebuilt during initialize.

this should not be necessary and is a performance issue.


Actual results:

a window & compositor was built, torn down, then rebuilt during mozilla initialize, using a simple page to test.


Expected results:

one nsWindow with associated compositor backend should have been created.

Comment 1

[Steve Williams]

The culprit is here :

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp

NS_IMETHODIMP nsDocumentViewer::Show(void)
{
	NS_ENSURE_TRUE(mDocument, NS_ERROR_NOT_AVAILABLE);
	// We don't need the previous viewer anymore since we're not
	// displaying it.
		if (mPreviousViewer) {
		// This little dance *may* only be to keep
		// PresShell::EndObservingDocument happy, but I'm not sure.
		nsCOMPtr<nsIContentViewer> prevViewer(mPreviousViewer);
		mPreviousViewer = nullptr;
		prevViewer->Destroy();


... that results in a complete teardown & rebuild of the compositor back end for no good reason that I'm aware of.

that "little dance" needs commenting properly or ripping out as it results in an undesired performance hit and initialization complexity.

Comment 2

[Steve Williams]

candidate patch :

[mozilla]/layout/base/nsDocumentViewer.cpp

	NS_IMETHODIMP
	nsDocumentViewer::Show(void)
	{
		NS_ENSURE_TRUE(mDocument, NS_ERROR_NOT_AVAILABLE);
		// Early out ... no need to do anything if the window is already visible.
		if (mWindow && mWindow->IsVisible())
			return NS_OK;

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Note the blame on that predates CVS->Mercurial switchover, so this is old code.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

jet - who should he work with to move forward on this?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So mPreviousViewer represents a state where, during the early parts of loading a document, we've got enough of the new document to construct a bunch of objects, but we're still displaying the previous document.  We set up the new document as the primary object because code depended on that for the new document to load correctly, but then mPreviousViewer is a pointer to the document and presentation that we're actually displaying right now.

So when it's actually time to show the new document, we should certainly destroy the previous document.

I'm wondering if the complaint is that we're setting up an about:blank document viewer when that's unnecessary?

What's the definition of "mozilla initialize" you're talking about?  Opening the first browser window?  Opening a new browser window?  Loading a new document in a window?

Comment 6

[Steve Williams]

hey david - yeah ... got that. 

> I'm wondering if the complaint is that we're setting up an about:blank document viewer when that's unnecessary?

bingo.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you verify in some way that the mPreviousViewer was for about:blank?

Comment 8

[Steve Williams]

yeah. got a patch here. give me a few minutes and you can review if you like.

Comment 9

[Steve Williams]

[mozilla]/docshell/base/nsDSURIContentListener.cpp

else 
{
        rv = mDocShell->CreateContentViewer(aContentType, request, aContentHandler);

->

else if (mDocShell->mHasLoadedNonBlankURI)

{
        rv = mDocShell->CreateContentViewer(aContentType, request, aContentHandler);


AND

[mozilla]/layout/base/nsDocumentViewer.cpp


       // Now that the document has loaded, we can tell the presshell
       // to unsuppress painting.
       if (mPresShell) {
         nsCOMPtr<nsIPresShell> shellDeathGrip(mPresShell);
         mPresShell->UnsuppressPainting();
         // mPresShell could have been removed now, see bug 378682/421432
         if (mPresShell) {
           mPresShell->LoadComplete();
         }
       }

--->


    // Don't create a viewer until we have something to display.
    // This prevents the initial Stop following initial URI navigate from creating a viewer/compositor backend.
    if (mDocument && (mDocument->GetReadyStateEnum() == nsIDocument::READYSTATE_COMPLETE))
    {
       // Now that the document has loaded, we can tell the presshell
       // to unsuppress painting.
       if (mPresShell) {
         nsCOMPtr<nsIPresShell> shellDeathGrip(mPresShell);
         mPresShell->UnsuppressPainting();
         // mPresShell could have been removed now, see bug 378682/421432
         if (mPresShell) {
           mPresShell->LoadComplete();
         }
       }
    }


sorry, that's not diff format but not working from trunk at the moment.

work in progress so not 100% sure of this yet.

Comment 10

[Steve Williams]

no. that's wrong. mHasLoadedNonBlankURI doesn't change. looking at it again.

Comment 11

[Steve Williams]

my sincere apologies. 

embarrassed to say that this turned out to be a duplicate LoadURI call from our embedding application.

very sorry to waste everyone's time. 

the two above *might* still be worthwhile optimizations but probably best to just close this to avoid unnecessary wild goose chases.