From 6c172e4b87f2d9a3d781c07da75dbb542d46007d Mon Sep 17 00:00:00 2001 From: Mohan Maiya Date: Sat, 20 Nov 2021 16:27:14 -0800 Subject: [PATCH] Add support for memory cleanup on process exit This patch adds a callback to cleanup memory on process exit. Bug: angleproject:6723 Test: Android CTS WrapperTest.testThreadCleanup Change-Id: Ia517d4c6ae280ddc1f17a3b6f77d437aaaad0678 Reviewed-on: https://chromium-review.googlesource.com/c/angle/angle/+/3294581 Reviewed-by: Jamie Madill Reviewed-by: Tim Van Patten Reviewed-by: Shahbaz Youssefi Commit-Queue: Mohan Maiya --- src/common/tls.cpp | 9 +- src/common/tls.h | 6 +- src/compiler/translator/PoolAlloc.cpp | 2 +- src/libANGLE/Context.cpp | 2 +- src/libANGLE/Display.cpp | 37 +++++++- src/libANGLE/Display.h | 15 +++- src/libANGLE/Thread.cpp | 19 +++++ src/libANGLE/Thread.h | 5 ++ src/libGLESv2/egl_stubs.cpp | 4 +- src/libGLESv2/global_state.cpp | 19 ++++- src/libGLESv2/global_state.h | 2 +- src/tests/egl_tests/EGLMultiContextTest.cpp | 95 +++++++++++++++++++++ 12 files changed, 195 insertions(+), 20 deletions(-) diff --git a/src/common/tls.cpp b/src/common/tls.cpp index 96b93e438..4f1fca5c9 100644 --- a/src/common/tls.cpp +++ b/src/common/tls.cpp @@ -36,7 +36,7 @@ static vector freeTlsIndices; bool gUseAndroidOpenGLTlsSlot = false; -TLSIndex CreateTLSIndex() +TLSIndex CreateTLSIndex(PthreadKeyDestructor destructor) { TLSIndex index; @@ -57,15 +57,14 @@ TLSIndex CreateTLSIndex() # endif #elif defined(ANGLE_PLATFORM_POSIX) - // Create global pool key - if ((pthread_key_create(&index, nullptr)) != 0) + // Create pthread key + if ((pthread_key_create(&index, destructor)) != 0) { index = TLS_INVALID_INDEX; } #endif - ASSERT(index != TLS_INVALID_INDEX && - "CreateTLSIndex(): Unable to allocate Thread Local Storage"); + ASSERT(index != TLS_INVALID_INDEX && "CreateTLSIndex: Unable to allocate Thread Local Storage"); return index; } diff --git a/src/common/tls.h b/src/common/tls.h index f234c8357..4075f3c03 100644 --- a/src/common/tls.h +++ b/src/common/tls.h @@ -44,10 +44,8 @@ typedef pthread_key_t TLSIndex; # error Unsupported platform. #endif -// TODO(kbr): for POSIX platforms this will have to be changed to take -// in a destructor function pointer, to allow the thread-local storage -// to be properly deallocated upon thread exit. -TLSIndex CreateTLSIndex(); +using PthreadKeyDestructor = void (*)(void *); +TLSIndex CreateTLSIndex(PthreadKeyDestructor destructor); bool DestroyTLSIndex(TLSIndex index); bool SetTLSValue(TLSIndex index, void *value); diff --git a/src/compiler/translator/PoolAlloc.cpp b/src/compiler/translator/PoolAlloc.cpp index d4a631b2d..1d770c622 100644 --- a/src/compiler/translator/PoolAlloc.cpp +++ b/src/compiler/translator/PoolAlloc.cpp @@ -15,7 +15,7 @@ bool InitializePoolIndex() { ASSERT(PoolIndex == TLS_INVALID_INDEX); - PoolIndex = CreateTLSIndex(); + PoolIndex = CreateTLSIndex(nullptr); return PoolIndex != TLS_INVALID_INDEX; } diff --git a/src/libANGLE/Context.cpp b/src/libANGLE/Context.cpp index cfb554189..04bf084ab 100644 --- a/src/libANGLE/Context.cpp +++ b/src/libANGLE/Context.cpp @@ -329,7 +329,7 @@ static TLSIndex GetCurrentValidContextTLSIndex() static dispatch_once_t once; dispatch_once(&once, ^{ ASSERT(CurrentValidContextIndex == TLS_INVALID_INDEX); - CurrentValidContextIndex = CreateTLSIndex(); + CurrentValidContextIndex = CreateTLSIndex(nullptr); }); return CurrentValidContextIndex; } diff --git a/src/libANGLE/Display.cpp b/src/libANGLE/Display.cpp index d49aedfea..a446841ca 100644 --- a/src/libANGLE/Display.cpp +++ b/src/libANGLE/Display.cpp @@ -753,6 +753,27 @@ Display *Display::GetDisplayFromDevice(Device *device, const AttributeMap &attri return display; } +// static +Display::EglDisplaySet Display::GetEglDisplaySet() +{ + Display::EglDisplaySet displays; + + ANGLEPlatformDisplayMap *anglePlatformDisplays = GetANGLEPlatformDisplayMap(); + DevicePlatformDisplayMap *devicePlatformDisplays = GetDevicePlatformDisplayMap(); + + for (auto anglePlatformDisplayMapEntry : *anglePlatformDisplays) + { + displays.insert(anglePlatformDisplayMapEntry.second); + } + + for (auto devicePlatformDisplayMapEntry : *devicePlatformDisplays) + { + displays.insert(devicePlatformDisplayMapEntry.second); + } + + return displays; +} + Display::Display(EGLenum platform, EGLNativeDisplayType displayId, Device *eglDevice) : mState(displayId), mImplementation(nullptr), @@ -984,7 +1005,7 @@ Error Display::initialize() return NoError(); } -Error Display::terminate(Thread *thread) +Error Display::terminate(Thread *thread, TerminateReason terminateReason) { mIsTerminated = true; @@ -999,11 +1020,19 @@ Error Display::terminate(Thread *thread) // thread, then they are not actually destroyed while they remain current. If other resources // created with respect to dpy are in use by any current context or surface, then they are also // not destroyed until the corresponding context or surface is no longer current. - for (const gl::Context *context : mContextSet) + for (gl::Context *context : mContextSet) { if (context->getRefCount() > 0) { - return NoError(); + if (terminateReason == TerminateReason::ProcessExit) + { + context->release(); + (void)context->unMakeCurrent(this); + } + else + { + return NoError(); + } } } @@ -1595,7 +1624,7 @@ Error Display::destroyContext(Thread *thread, gl::Context *context) } } - return terminate(thread); + return terminate(thread, TerminateReason::InternalCleanup); } return NoError(); diff --git a/src/libANGLE/Display.h b/src/libANGLE/Display.h index d4615c7f8..15a302930 100644 --- a/src/libANGLE/Display.h +++ b/src/libANGLE/Display.h @@ -128,7 +128,17 @@ class Display final : public LabeledObject, void onSubjectStateChange(angle::SubjectIndex index, angle::SubjectMessage message) override; Error initialize(); - Error terminate(Thread *thread); + + enum class TerminateReason + { + Api, + InternalCleanup, + ProcessExit, + + InvalidEnum, + EnumCount = InvalidEnum, + }; + Error terminate(Thread *thread, TerminateReason terminateReason); // Called before all display state dependent EGL functions. Backends can set up, for example, // thread-specific backend state through this function. Not called for functions that do not // need the state. @@ -142,6 +152,9 @@ class Display final : public LabeledObject, const AttributeMap &attribMap); static Display *GetExistingDisplayFromNativeDisplay(EGLNativeDisplayType nativeDisplay); + using EglDisplaySet = std::set; + static EglDisplaySet GetEglDisplaySet(); + static const ClientExtensions &GetClientExtensions(); static const std::string &GetClientExtensionString(); diff --git a/src/libANGLE/Thread.cpp b/src/libANGLE/Thread.cpp index 6726a4543..6c8bda626 100644 --- a/src/libANGLE/Thread.cpp +++ b/src/libANGLE/Thread.cpp @@ -10,11 +10,30 @@ #include "libANGLE/Context.h" #include "libANGLE/Debug.h" +#include "libANGLE/Display.h" #include "libANGLE/Error.h" namespace angle { bool gUseAndroidOpenGLTlsSlot; +std::atomic_int gProcessCleanupRefCount(0); + +void ProcessCleanupCallback(void *ptr) +{ + egl::Thread *thread = static_cast(ptr); + ASSERT(thread); + + ASSERT(gProcessCleanupRefCount > 0); + if (--gProcessCleanupRefCount == 0) + { + egl::Display::EglDisplaySet displays = egl::Display::GetEglDisplaySet(); + for (egl::Display *display : displays) + { + ASSERT(display); + (void)display->terminate(thread, egl::Display::TerminateReason::ProcessExit); + } + } +} } // namespace angle namespace egl diff --git a/src/libANGLE/Thread.h b/src/libANGLE/Thread.h index d54708837..f8a159f3e 100644 --- a/src/libANGLE/Thread.h +++ b/src/libANGLE/Thread.h @@ -13,9 +13,14 @@ #include "libANGLE/Debug.h" +#include + namespace angle { extern bool gUseAndroidOpenGLTlsSlot; +extern std::atomic_int gProcessCleanupRefCount; + +void ProcessCleanupCallback(void *ptr); } // namespace angle namespace gl diff --git a/src/libGLESv2/egl_stubs.cpp b/src/libGLESv2/egl_stubs.cpp index 7d091044d..fa37ae68e 100644 --- a/src/libGLESv2/egl_stubs.cpp +++ b/src/libGLESv2/egl_stubs.cpp @@ -663,8 +663,8 @@ EGLBoolean Terminate(Thread *thread, Display *display) ScopedSyncCurrentContextFromThread scopedSyncCurrent(thread); - ANGLE_EGL_TRY_RETURN(thread, display->terminate(thread), "eglTerminate", - GetDisplayIfValid(display), EGL_FALSE); + ANGLE_EGL_TRY_RETURN(thread, display->terminate(thread, Display::TerminateReason::Api), + "eglTerminate", GetDisplayIfValid(display), EGL_FALSE); thread->setSuccess(); return EGL_TRUE; diff --git a/src/libGLESv2/global_state.cpp b/src/libGLESv2/global_state.cpp index 12857f28f..f3d11f680 100644 --- a/src/libGLESv2/global_state.cpp +++ b/src/libGLESv2/global_state.cpp @@ -63,6 +63,23 @@ Thread *AllocateCurrentThread() #else gl::gCurrentValidContext = nullptr; #endif + +#if defined(ANGLE_PLATFORM_ANDROID) + static pthread_once_t keyOnce = PTHREAD_ONCE_INIT; + static TLSIndex gProcessCleanupTLSIndex = TLS_INVALID_INDEX; + + // Create process cleanup TLS slot + auto CreateProcessCleanupTLSIndex = []() { + gProcessCleanupTLSIndex = CreateTLSIndex(angle::ProcessCleanupCallback); + }; + pthread_once(&keyOnce, CreateProcessCleanupTLSIndex); + ASSERT(gProcessCleanupTLSIndex != TLS_INVALID_INDEX); + + // Initialize process cleanup TLS slot + angle::gProcessCleanupRefCount++; + SetTLSValue(gProcessCleanupTLSIndex, thread); +#endif // ANGLE_PLATFORM_ANDROID + ASSERT(thread); return thread; } @@ -94,7 +111,7 @@ static TLSIndex GetCurrentThreadTLSIndex() static dispatch_once_t once; dispatch_once(&once, ^{ ASSERT(CurrentThreadIndex == TLS_INVALID_INDEX); - CurrentThreadIndex = CreateTLSIndex(); + CurrentThreadIndex = CreateTLSIndex(nullptr); }); return CurrentThreadIndex; } diff --git a/src/libGLESv2/global_state.h b/src/libGLESv2/global_state.h index 1d66cb5a6..4f7454297 100644 --- a/src/libGLESv2/global_state.h +++ b/src/libGLESv2/global_state.h @@ -14,7 +14,7 @@ #include "libANGLE/Thread.h" #include "libANGLE/features.h" -#if defined(ANGLE_PLATFORM_APPLE) +#if defined(ANGLE_PLATFORM_APPLE) || (ANGLE_PLATFORM_ANDROID) # include "common/tls.h" #endif diff --git a/src/tests/egl_tests/EGLMultiContextTest.cpp b/src/tests/egl_tests/EGLMultiContextTest.cpp index 0685563a9..00986bf68 100644 --- a/src/tests/egl_tests/EGLMultiContextTest.cpp +++ b/src/tests/egl_tests/EGLMultiContextTest.cpp @@ -53,6 +53,56 @@ class EGLMultiContextTest : public ANGLETest getEGLWindow()->makeCurrent(); } + bool chooseConfig(EGLDisplay dpy, EGLConfig *config) const + { + bool result = false; + EGLint count = 0; + EGLint clientVersion = EGL_OPENGL_ES3_BIT; + EGLint attribs[] = {EGL_RED_SIZE, + 8, + EGL_GREEN_SIZE, + 8, + EGL_BLUE_SIZE, + 8, + EGL_ALPHA_SIZE, + 8, + EGL_RENDERABLE_TYPE, + clientVersion, + EGL_SURFACE_TYPE, + EGL_WINDOW_BIT, + EGL_NONE}; + + result = eglChooseConfig(dpy, attribs, config, 1, &count); + EXPECT_EGL_TRUE(result && (count > 0)); + return result; + } + + bool createContext(EGLDisplay dpy, EGLConfig config, EGLContext *context) + { + bool result = false; + EGLint attribs[] = {EGL_CONTEXT_MAJOR_VERSION, 3, EGL_NONE}; + + *context = eglCreateContext(dpy, config, nullptr, attribs); + result = (*context != EGL_NO_CONTEXT); + EXPECT_TRUE(result); + return result; + } + + bool createPbufferSurface(EGLDisplay dpy, + EGLConfig config, + EGLint width, + EGLint height, + EGLSurface *surface) + { + bool result = false; + EGLint attribs[] = {EGL_WIDTH, width, EGL_HEIGHT, height, EGL_NONE}; + + *surface = eglCreatePbufferSurface(dpy, config, attribs); + result = (*surface != EGL_NO_SURFACE); + EXPECT_TRUE(result); + return result; + } + EGLContext mContexts[2]; GLuint mTexture; }; @@ -238,6 +288,51 @@ void main() eglDestroyContext(dpy, ctx[t]); } } + +// Test that repeated EGL init + terminate with improper cleanup doesn't cause an OOM crash. +// To reproduce the memleak issue changes need to be made to "EGLWindow::destroyGL" as shown here -> +// https://chromium-review.googlesource.com/c/angle/angle/+/3294581/5/util/EGLWindow.cpp +TEST_P(EGLMultiContextTest, RepeatedEglInitAndTerminate) +{ + ANGLE_SKIP_TEST_IF(!IsAndroid() || !IsVulkan()); + + EGLDisplay dpy; + EGLSurface srf; + EGLContext ctx; + EGLint dispattrs[] = {EGL_PLATFORM_ANGLE_TYPE_ANGLE, GetParam().getRenderer(), EGL_NONE}; + + for (int i = 0; i < 100; i++) + { + std::thread thread = std::thread([&]() { + dpy = eglGetPlatformDisplayEXT( + EGL_PLATFORM_ANGLE_ANGLE, reinterpret_cast(EGL_DEFAULT_DISPLAY), dispattrs); + EXPECT_TRUE(dpy != EGL_NO_DISPLAY); + EXPECT_EGL_TRUE(eglInitialize(dpy, nullptr, nullptr)); + + EGLConfig config = EGL_NO_CONFIG_KHR; + EXPECT_TRUE(chooseConfig(dpy, &config)); + + EXPECT_TRUE(createPbufferSurface(dpy, config, 2560, 1080, &srf)); + ASSERT_EGL_SUCCESS() << "eglCreatePbufferSurface failed."; + + EXPECT_TRUE(createContext(dpy, config, &ctx)); + EXPECT_EGL_TRUE(eglMakeCurrent(dpy, srf, srf, ctx)); + + // Clear and read back to make sure thread uses context. + glClearColor(1.0, 0.0, 0.0, 1.0); + glClear(GL_COLOR_BUFFER_BIT); + EXPECT_PIXEL_EQ(0, 0, 255, 0, 0, 255); + + eglTerminate(dpy); + EXPECT_EGL_SUCCESS(); + dpy = EGL_NO_DISPLAY; + srf = EGL_NO_SURFACE; + ctx = EGL_NO_CONTEXT; + }); + + thread.join(); + } +} } // anonymous namespace GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(EGLMultiContextTest);