Skip to content

SL-18721: Faster viewer shutdown time since performance improvements can lead to perceived inventory loss due to cache corruption#110

Merged
LLGuru merged 1 commit intoDRTVWR-580-maint-Tfrom
SL-18721
Mar 20, 2023
Merged

SL-18721: Faster viewer shutdown time since performance improvements can lead to perceived inventory loss due to cache corruption#110
LLGuru merged 1 commit intoDRTVWR-580-maint-Tfrom
SL-18721

Conversation

@LLGuru
Copy link
Contributor

@LLGuru LLGuru commented Mar 6, 2023

Make LLWindowWin32::LLWindowWin32Thread::close() to be called from the class destructor

…can lead to perceived inventory loss due to cache corruption
@LLGuru LLGuru changed the title SL-18721: Disconnect sooner to force the cache write SL-18721: Faster viewer shutdown time since performance improvements can lead to perceived inventory loss due to cache corruption Mar 20, 2023
@LLGuru LLGuru requested a review from nat-goodspeed March 20, 2023 09:32

// Special workflow for LLWindowWin32Thread - it's close() should be called explicitly
if (mExplicitShutdown)
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What issues close() call in such case?

Copy link
Contributor Author

@LLGuru LLGuru Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What issues close() call in such case?

As iit was said in the comment above, the destructor of the class ThreadPool calls this method in any case:

LL::ThreadPool::~ThreadPool()
{
    close();
}

@LLGuru LLGuru requested a review from akleshchev March 20, 2023 15:22
Copy link
Contributor

@nat-goodspeed nat-goodspeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that you're deferring the LLWindowWin32Thread::close() call to its destructor, and that it's explicitly destroyed by ~LLWindowWin32(). But I don't know where the LLWindow subclass is destroyed?

@LLGuru
Copy link
Contributor Author

LLGuru commented Mar 20, 2023

I understand that you're deferring the LLWindowWin32Thread::close() call to its destructor, and that it's explicitly destroyed by ~LLWindowWin32(). But I don't know where the LLWindow subclass is destroyed?

The class LLWindow destructor is being explicitly called from LLWindowManager::destroyWindow()

BOOL LLWindowManager::destroyWindow(LLWindow* window)
{
	if (sWindowList.find(window) == sWindowList.end())
	{
		LL_ERRS() << "LLWindowManager::destroyWindow() : Window pointer not valid, this window doesn't exist!" 
			<< LL_ENDL;
		return FALSE;
	}

	window->close();

	sWindowList.erase(window);

	delete window;

	return TRUE;
}

The method LLWindowManager::destroyWindow() is being called from LLViewerWindow::destroyWindow()

void LLViewerWindow::destroyWindow()
{
	if (mWindow)
	{
		LLWindowManager::destroyWindow(mWindow);
	}
	mWindow = NULL;
}

The method LLViewerWindow::destroyWindow() is being called from LLViewerWindow::~LLViewerWindow()

LLViewerWindow::~LLViewerWindow()
{
	LL_INFOS() << "Destroying Window" << LL_ENDL;
	destroyWindow();

	delete mDebugText;
	mDebugText = NULL;

	if (LLViewerShaderMgr::sInitialized)
	{
		LLViewerShaderMgr::releaseInstance();
		LLViewerShaderMgr::sInitialized = FALSE;
	}
}

The full callstack looks as follows:

secondlife-bin.exe!LLWindow::~LLWindow() Line 125
[External Code]	
secondlife-bin.exe!LLWindowManager::destroyWindow(LLWindow * window) Line 465
[Inline Frame] secondlife-bin.exe!LLViewerWindow::destroyWindow() Line 5353
secondlife-bin.exe!LLViewerWindow::~LLViewerWindow() Line 2458
[External Code]	
secondlife-bin.exe!LLAppViewer::cleanup() Line 1903
secondlife-bin.exe!LLAppViewerWin32::cleanup() Line 705
secondlife-bin.exe!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * pCmdLine, int nCmdShow) Line 464

@nat-goodspeed nat-goodspeed self-requested a review March 20, 2023 19:38
Copy link
Contributor

@nat-goodspeed nat-goodspeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I thought we'd need a new call at shutdown time, but your answer addresses my concern.

@LLGuru LLGuru merged commit cf692c4 into DRTVWR-580-maint-T Mar 20, 2023
@LLGuru LLGuru deleted the SL-18721 branch March 20, 2023 20:08
@github-actions github-actions bot locked and limited conversation to collaborators Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants