Minimal change to get Chakra consistently shutting down properly - #146
Conversation
…n properly. Contains some...questionable options.
| { | ||
| Log(L"Failed to dispose runtime with error code (%u)\n", errorCode); | ||
| } | ||
| ThrowIfFailed(JsSetCurrentContext(JS_INVALID_REFERENCE)); |
There was a problem hiding this comment.
First and least controversial change, allows Chakra to shut down correctly. I have not yet confirmed that this resolves the memory leak we were seeing before as there's a problem I just found with refresh; I'll update this comment when I fix that and confirm (or otherwise).
There was a problem hiding this comment.
Fixed the problems with refresh. Tested to confirm that refresh works, and has consistent and sane memory behavior, on Win32-Chakra, Win32-V8, and UWP-Chakra. UWP-V8 builds but does not run, which seems to be no change from master.
There was a problem hiding this comment.
UWP-V8 builds but does not run
Do we have an issue for this?
| @@ -33,6 +33,37 @@ namespace Babylon | |||
| { | |||
| namespace | |||
| { | |||
There was a problem hiding this comment.
Definitely the weirdest "fix" in here, and the one I'm least sure about. bgfx doesn't seem to have a mechanism for asking it whether it's alive or not, but some JavaScript-owned objects (relevantly the ProgramData) can end up being destroyed after NativeEngine has already been destroyed and bgfx::shutdown has consequently already been called. They could also theoretically survive long enough to last into another bgfx initialization (if that's possible), which would be disastrous as their handles would now point to things inside bgfx that were really created and owned by somebody else. This global state holder was created to keep track of this. I'm not a fan of it, however, so please let me know of whatever other ideas you'd like to discuss.
There was a problem hiding this comment.
There are 2 resources to free here : the bgfx shader handle and the JS object. I'd try to keep a list of ProgramData (register in ctor, unregister in dtor). When bgfx::shutdown is called, I'd run thru all the registered ProgramData and free their bgfx handle. I'd then let JS engine free the ProgramData when ever it needs to. I think the same mechanism happens with TextureData. I prefer to be pro active in destruction than trying to determine a state(m_isBgfxInitialized/m_bgfxInitializationId) where I can or can't do something. The more states, the more potential bugs.
There was a problem hiding this comment.
Definitely agree that that's cleaner, and it seems the ticketed mechanism can be adapted for this purpose. I've refactored to try this other approach.
| void Resize(size_t newWidth, size_t newHeight); | ||
|
|
||
| using OnResizeCallback = std::function<void(size_t, size_t)>; | ||
| using OnResizeCallbackTicket = arcana::ticketed_collection<OnResizeCallback>::ticket; |
There was a problem hiding this comment.
The third and middle-est change removes the dependency on Arcana's ticketed_collection and just makes a small bespoke alternative instead. This was to get around the order-of-destruction problem described in #142, which we should still consider how to deal with long-term but may not be as tricky as I thought at first. In this particular case, all that's necessary to allow the NativeWindow and NativeEngine to be destroyed in arbitrary orders is for the OnResizeCallbackTicket to simply do nothing if the collection it came from was destroyed before it was. arcana::ticketed_collection doesn't support this, and adding that is a work-in-progress due to the fact that ticketed_collection is supposed to be thread-safe. But thread-safety isn't a concern here as these are both Babylon Native plugins that should only talk to each other on the same thread, so we can get away with doing something simpler. (And we can always move back to the Arcana type when it's ready, if we want to.)
…emory behavior (on the default experience) on Win32-Chakra, Win32-V8, and UWP-Chakra. UWP-V8 builds but is unable to start, which represents no change from master.
| }; | ||
|
|
||
| return Napi::External<ProgramData>::New(info.Env(), programData, finalizer); | ||
| auto* programDataPtr = programData.get(); |
There was a problem hiding this comment.
rawProgramData
| { | ||
| Log(L"Failed to dispose runtime with error code (%u)\n", errorCode); | ||
| } | ||
| ThrowIfFailed(JsSetCurrentContext(JS_INVALID_REFERENCE)); |
There was a problem hiding this comment.
UWP-V8 builds but does not run
Do we have an issue for this?
| m_collection.erase(m_isCollectionStillAlive); | ||
| } | ||
|
|
||
| delete m_isCollectionStillAlive; |
| { | ||
| // NOTE: This type is not thread-safe. | ||
| template<typename T> | ||
| class ticketed_collection |
There was a problem hiding this comment.
should prevent copy and move semantics
| bool* m_isCollectionStillAlive{}; | ||
| MapT& m_collection; |
There was a problem hiding this comment.
We discussed this offline. I would recommend using a double pointer here instead.
371d90098b Use zero depth to mean not a volume texture. (BabylonJS#150) b8cef564e3 texturec: Added positional arguments. (BabylonJS#149) 3b4baab012 Add BC4S, BC5S, BC6HU, RGB10A2U and D32FS8 texture formats. (BabylonJS#148) 0a64629d9f Updated tintexr. (BabylonJS#147) e95c449452 Updated stb_image. (BabylonJS#146) da38bface6 Fixed bounds check, and EXR channel pixel type. (BabylonJS#145) be16d98e34 Updated simplewebp. (BabylonJS#144) git-subtree-dir: Dependencies/bimg git-subtree-split: 371d90098b1fd017cd00205979d5ef74b8c3ed62
3 participants
Contains some...questionable options. Not sure if this should be checked in or not, but it's probably worth discussing at least. There are fixes in here for three separate problems that were causing Chakra to either fail to shut down or to shut down improperly. I'll call each of the problems/fixes out in comments and explain whether/why iffy about the alternative presented.