From 612f08ed1f18e138ab520dec67a6d1f4cb643c9f Mon Sep 17 00:00:00 2001 From: Oliver Bock Date: Mon, 3 Aug 2020 17:12:28 +1000 Subject: [PATCH 1/2] Clean up external object references when they are garbage collected, rather than when the context is destroyed. --- .../Noesis.Javascript/JavascriptContext.cpp | 30 +++++++++---------- Source/Noesis.Javascript/JavascriptContext.h | 13 ++++---- .../Noesis.Javascript/JavascriptExternal.cpp | 9 +++--- Source/Noesis.Javascript/JavascriptExternal.h | 3 -- .../Noesis.Javascript/JavascriptInterop.cpp | 21 ++++++++++--- 5 files changed, 42 insertions(+), 34 deletions(-) diff --git a/Source/Noesis.Javascript/JavascriptContext.cpp b/Source/Noesis.Javascript/JavascriptContext.cpp index 2372ebb..73c5fbc 100644 --- a/Source/Noesis.Javascript/JavascriptContext.cpp +++ b/Source/Noesis.Javascript/JavascriptContext.cpp @@ -171,8 +171,9 @@ JavascriptContext::JavascriptContext() isolate->SetFatalErrorHandler(FatalErrorCallback); - mExternals = gcnew System::Collections::Generic::Dictionary(); mTypeToConstructorMapping = gcnew System::Collections::Generic::Dictionary(); + mTypeToMethods = gcnew System::Collections::Generic::Dictionary^>(); + mFunctions = gcnew System::Collections::Generic::List(); HandleScope scope(isolate); mContext = new Persistent(isolate, Context::New(isolate)); @@ -186,8 +187,6 @@ JavascriptContext::~JavascriptContext() { v8::Locker v8ThreadLock(isolate); v8::Isolate::Scope isolate_scope(isolate); - for each (WrappedJavascriptExternal wrapped in mExternals->Values) - delete wrapped.Pointer; for each (System::WeakReference^ f in mFunctions) { JavascriptFunction ^function = safe_cast(f->Target); if (function != nullptr) @@ -197,8 +196,8 @@ JavascriptContext::~JavascriptContext() delete (void *)p; } delete mContext; - delete mExternals; delete mTypeToConstructorMapping; + delete mTypeToMethods; delete mFunctions; } if (isolate != NULL) @@ -298,6 +297,7 @@ void JavascriptContext::SetConstructor(System::String^ name, System::Type^ assoc functionTemplate->SetClassName(className); JavascriptInterop::InitObjectWrapperTemplate(functionTemplate->InstanceTemplate()); mTypeToConstructorMapping[associatedType] = System::IntPtr(new Persistent(isolate, functionTemplate)); + mTypeToMethods[associatedType] = gcnew System::Collections::Generic::Dictionary(); Local::New(isolate, *mContext)->Global()->Set(context, className, functionTemplate->GetFunction(context).ToLocalChecked()); } @@ -505,18 +505,7 @@ JavascriptContext::Collect() JavascriptExternal* JavascriptContext::WrapObject(System::Object^ iObject) { - WrappedJavascriptExternal external_wrapped; - if (mExternals->TryGetValue(iObject, external_wrapped)) - { - // We've wrapped this guy before. - return external_wrapped.Pointer; - } - else - { - JavascriptExternal* external = new JavascriptExternal(iObject); - mExternals[iObject] = WrappedJavascriptExternal(external); - return external; - } + return new JavascriptExternal(iObject); } //////////////////////////////////////////////////////////////////////////////////////////////////// @@ -529,6 +518,7 @@ JavascriptContext::GetObjectWrapperConstructorTemplate(System::Type ^type) Local constructor = FunctionTemplate::New(GetCurrentIsolate()); JavascriptInterop::InitObjectWrapperTemplate(constructor->InstanceTemplate()); mTypeToConstructorMapping[type] = System::IntPtr(new Persistent(isolate, constructor)); + mTypeToMethods[type] = gcnew System::Collections::Generic::Dictionary(); return constructor; } Persistent *constructor = (Persistent *)(void *)ptrToConstructor; @@ -537,6 +527,14 @@ JavascriptContext::GetObjectWrapperConstructorTemplate(System::Type ^type) //////////////////////////////////////////////////////////////////////////////////////////////////// +System::Collections::Generic::Dictionary^ +JavascriptContext::MethodsForType(System::Type^ type) +{ + return mTypeToMethods[type]; +} + +//////////////////////////////////////////////////////////////////////////////////////////////////// + void JavascriptContext::RegisterFunction(System::Object^ f) { diff --git a/Source/Noesis.Javascript/JavascriptContext.h b/Source/Noesis.Javascript/JavascriptContext.h index 553a6c8..4c40c91 100644 --- a/Source/Noesis.Javascript/JavascriptContext.h +++ b/Source/Noesis.Javascript/JavascriptContext.h @@ -94,6 +94,7 @@ public value struct WrappedJavascriptExternal { private: System::IntPtr pointer; + System::Type^ type; internal: WrappedJavascriptExternal(JavascriptExternal *value) @@ -196,6 +197,8 @@ public ref class JavascriptContext: public System::IDisposable Local GetObjectWrapperConstructorTemplate(System::Type ^type); + System::Collections::Generic::Dictionary^ MethodsForType(System::Type^ type); + void RegisterFunction(System::Object^ f); static void FatalErrorCallbackMember(const char* location, const char* message); @@ -212,18 +215,16 @@ public ref class JavascriptContext: public System::IDisposable // v8 context required to be active for all v8 operations. Persistent* mContext; - // Stores every JavascriptExternal we create. This saves time if the same - // objects are recreated frequently, and stops us building up a huge - // collection of JavascriptExternal objects that won't be freed until - // the context is destroyed. - System::Collections::Generic::Dictionary ^mExternals; - // Maps types to their constructor function templates // The mapping will either be defined by the user calling `SetConstructor` or autogenerated if no // mapping was provided. // The `IntPtr` points to a `Persistent`. System::Collections::Generic::Dictionary ^mTypeToConstructorMapping; + // For each distinct CLR type we've wrapped, this accumulates functions + // constructed to access its methods/properties. + System::Collections::Generic::Dictionary^>^ mTypeToMethods; + // Stores every JavascriptFunction we create. Ensures we dispose of them // all. System::Collections::Generic::List ^mFunctions; diff --git a/Source/Noesis.Javascript/JavascriptExternal.cpp b/Source/Noesis.Javascript/JavascriptExternal.cpp index 378ffdb..79aa35b 100644 --- a/Source/Noesis.Javascript/JavascriptExternal.cpp +++ b/Source/Noesis.Javascript/JavascriptExternal.cpp @@ -52,7 +52,6 @@ JavascriptExternal::JavascriptExternal(System::Object^ iObject) { mObjectHandle = System::Runtime::InteropServices::GCHandle::Alloc(iObject); mOptions = SetParameterOptions::None; - mMethods = gcnew System::Collections::Generic::Dictionary(); } //////////////////////////////////////////////////////////////////////////////////////////////////// @@ -76,15 +75,16 @@ Local JavascriptExternal::GetMethod(wstring iName) { v8::Isolate *isolate = JavascriptContext::GetCurrentIsolate(); - System::Collections::Generic::Dictionary ^methods = mMethods; + JavascriptContext^ context = JavascriptContext::GetCurrent(); + System::Object^ self = mObjectHandle.Target; + System::Type^ type = self->GetType(); + System::Collections::Generic::Dictionary ^methods = context->MethodsForType(type); System::String^ memberName = gcnew System::String(iName.c_str()); WrappedMethod method; if (methods->TryGetValue(memberName, method)) return Local::New(isolate, *(method.Pointer)); else { - System::Object^ self = mObjectHandle.Target; - System::Type^ type = self->GetType(); System::String^ memberName = gcnew System::String(iName.c_str()); cli::array^ objectInfo = gcnew cli::array(2); objectInfo->SetValue(self,0); @@ -94,7 +94,6 @@ JavascriptExternal::GetMethod(wstring iName) cli::array^ members = type->GetMember(memberName); if (members->Length > 0 && members[0]->MemberType == MemberTypes::Method) { - JavascriptContext^ context = JavascriptContext::GetCurrent(); Local external = External::New(isolate, context->WrapObject(objectInfo)); Local functionTemplate = FunctionTemplate::New(isolate, JavascriptInterop::Invoker, external); Local function = functionTemplate->GetFunction(isolate->GetCurrentContext()).ToLocalChecked(); diff --git a/Source/Noesis.Javascript/JavascriptExternal.h b/Source/Noesis.Javascript/JavascriptExternal.h index 6eaf995..566aa21 100644 --- a/Source/Noesis.Javascript/JavascriptExternal.h +++ b/Source/Noesis.Javascript/JavascriptExternal.h @@ -98,9 +98,6 @@ class JavascriptExternal SetParameterOptions mOptions; - // Owned by JavascriptContext. - gcroot ^> mMethods; - std::unique_ptr> mIterator; static void IteratorCallback(const v8::FunctionCallbackInfo& iArgs); static void IteratorNextCallback(const v8::FunctionCallbackInfo& iArgs); diff --git a/Source/Noesis.Javascript/JavascriptInterop.cpp b/Source/Noesis.Javascript/JavascriptInterop.cpp index 89af423..98cf5fa 100644 --- a/Source/Noesis.Javascript/JavascriptInterop.cpp +++ b/Source/Noesis.Javascript/JavascriptInterop.cpp @@ -259,7 +259,14 @@ JavascriptInterop::ConvertToV8(System::Object^ iObject) //////////////////////////////////////////////////////////////////////////////////////////////////// -// TODO: should return Local +// a sort-of destructor in v8 land +void JavascriptExternalMakeWeak(const v8::WeakCallbackInfo& data) { + v8::Isolate* isolate = JavascriptContext::GetCurrentIsolate(); + JavascriptExternal *external = data.GetParameter(); + delete external; +} + +// TO DO: should return Local Local JavascriptInterop::WrapObject(System::Object^ iObject) { @@ -267,11 +274,17 @@ JavascriptInterop::WrapObject(System::Object^ iObject) if (context != nullptr) { - Local templ = context->GetObjectWrapperConstructorTemplate(iObject->GetType()); + System::Type ^object_type = iObject->GetType(); + Local templ = context->GetObjectWrapperConstructorTemplate(object_type); v8::Isolate *isolate = JavascriptContext::GetCurrentIsolate(); Local instanceTemplate = templ->InstanceTemplate(); Local object = instanceTemplate->NewInstance(isolate->GetCurrentContext()).ToLocalChecked(); - object->SetInternalField(0, External::New(isolate, context->WrapObject(iObject))); + JavascriptExternal* external = context->WrapObject(iObject); + object->SetInternalField(0, External::New(isolate, external)); + + // So we're notified when this object is no longer needed. + v8::Persistent> pobj(isolate, object); + pobj.SetWeak(external, JavascriptExternalMakeWeak, v8::WeakCallbackType::kParameter); return object; } @@ -281,7 +294,7 @@ JavascriptInterop::WrapObject(System::Object^ iObject) //////////////////////////////////////////////////////////////////////////////////////////////////// -// TODO: should use Local iExternal +// TO DO: should use Local iExternal System::Object^ JavascriptInterop::UnwrapObject(Local iValue) { From f5de7b8d5cb96c1f88755f26f1a6d4108d6491b2 Mon Sep 17 00:00:00 2001 From: Oliver Bock Date: Wed, 5 Aug 2020 12:01:08 +1000 Subject: [PATCH 2/2] Cache dictionary of methods with JavaScriptExternal to make Getter() faster. --- .../Noesis.Javascript/JavascriptContext.cpp | 2 +- Source/Noesis.Javascript/JavascriptContext.h | 3 +-- .../Noesis.Javascript/JavascriptExternal.cpp | 21 +++++++++++-------- Source/Noesis.Javascript/JavascriptExternal.h | 5 ++++- .../Noesis.Javascript/JavascriptInterop.cpp | 2 +- 5 files changed, 19 insertions(+), 14 deletions(-) diff --git a/Source/Noesis.Javascript/JavascriptContext.cpp b/Source/Noesis.Javascript/JavascriptContext.cpp index 73c5fbc..06ff19d 100644 --- a/Source/Noesis.Javascript/JavascriptContext.cpp +++ b/Source/Noesis.Javascript/JavascriptContext.cpp @@ -505,7 +505,7 @@ JavascriptContext::Collect() JavascriptExternal* JavascriptContext::WrapObject(System::Object^ iObject) { - return new JavascriptExternal(iObject); + return new JavascriptExternal(iObject, false); } //////////////////////////////////////////////////////////////////////////////////////////////////// diff --git a/Source/Noesis.Javascript/JavascriptContext.h b/Source/Noesis.Javascript/JavascriptContext.h index 4c40c91..8e81d0b 100644 --- a/Source/Noesis.Javascript/JavascriptContext.h +++ b/Source/Noesis.Javascript/JavascriptContext.h @@ -94,7 +94,6 @@ public value struct WrappedJavascriptExternal { private: System::IntPtr pointer; - System::Type^ type; internal: WrappedJavascriptExternal(JavascriptExternal *value) @@ -199,7 +198,7 @@ public ref class JavascriptContext: public System::IDisposable System::Collections::Generic::Dictionary^ MethodsForType(System::Type^ type); - void RegisterFunction(System::Object^ f); + void RegisterFunction(System::Object^ f); static void FatalErrorCallbackMember(const char* location, const char* message); diff --git a/Source/Noesis.Javascript/JavascriptExternal.cpp b/Source/Noesis.Javascript/JavascriptExternal.cpp index 79aa35b..ddbbaa4 100644 --- a/Source/Noesis.Javascript/JavascriptExternal.cpp +++ b/Source/Noesis.Javascript/JavascriptExternal.cpp @@ -48,10 +48,13 @@ using namespace std; //////////////////////////////////////////////////////////////////////////////////////////////////// -JavascriptExternal::JavascriptExternal(System::Object^ iObject) +JavascriptExternal::JavascriptExternal(System::Object^ iObject, bool needs_methods) { mObjectHandle = System::Runtime::InteropServices::GCHandle::Alloc(iObject); mOptions = SetParameterOptions::None; + System::Type^ type = iObject->GetType(); + if (needs_methods) + mMethods = JavascriptContext::GetCurrent()->MethodsForType(type); } //////////////////////////////////////////////////////////////////////////////////////////////////// @@ -75,17 +78,16 @@ Local JavascriptExternal::GetMethod(wstring iName) { v8::Isolate *isolate = JavascriptContext::GetCurrentIsolate(); - JavascriptContext^ context = JavascriptContext::GetCurrent(); - System::Object^ self = mObjectHandle.Target; - System::Type^ type = self->GetType(); - System::Collections::Generic::Dictionary ^methods = context->MethodsForType(type); - System::String^ memberName = gcnew System::String(iName.c_str()); + System::Collections::Generic::Dictionary^ methods = mMethods; + System::String^ memberName = gcnew System::String(iName.c_str()); WrappedMethod method; if (methods->TryGetValue(memberName, method)) return Local::New(isolate, *(method.Pointer)); else { - System::String^ memberName = gcnew System::String(iName.c_str()); + System::Object^ self = mObjectHandle.Target; + System::Type^ type = self->GetType(); + System::String^ memberName = gcnew System::String(iName.c_str()); cli::array^ objectInfo = gcnew cli::array(2); objectInfo->SetValue(self,0); objectInfo->SetValue(memberName,1); @@ -94,13 +96,14 @@ JavascriptExternal::GetMethod(wstring iName) cli::array^ members = type->GetMember(memberName); if (members->Length > 0 && members[0]->MemberType == MemberTypes::Method) { - Local external = External::New(isolate, context->WrapObject(objectInfo)); + JavascriptContext^ context = JavascriptContext::GetCurrent(); + Local external = External::New(isolate, context->WrapObject(objectInfo)); Local functionTemplate = FunctionTemplate::New(isolate, JavascriptInterop::Invoker, external); Local function = functionTemplate->GetFunction(isolate->GetCurrentContext()).ToLocalChecked(); Persistent *function_ptr = new Persistent(isolate, function); WrappedMethod wrapped(function_ptr); - methods[memberName] = wrapped; + methods[memberName] = wrapped; return function; } diff --git a/Source/Noesis.Javascript/JavascriptExternal.h b/Source/Noesis.Javascript/JavascriptExternal.h index 566aa21..5beaaca 100644 --- a/Source/Noesis.Javascript/JavascriptExternal.h +++ b/Source/Noesis.Javascript/JavascriptExternal.h @@ -57,7 +57,7 @@ class JavascriptExternal //////////////////////////////////////////////////////////// public: - JavascriptExternal(System::Object^ iObject); + JavascriptExternal(System::Object^ iObject, bool needs_methods); ~JavascriptExternal(); @@ -98,6 +98,9 @@ class JavascriptExternal SetParameterOptions mOptions; + // Owned by JavascriptContext. Only set when the object is with `JavascriptInterop::Getter()` + gcroot^> mMethods; + std::unique_ptr> mIterator; static void IteratorCallback(const v8::FunctionCallbackInfo& iArgs); static void IteratorNextCallback(const v8::FunctionCallbackInfo& iArgs); diff --git a/Source/Noesis.Javascript/JavascriptInterop.cpp b/Source/Noesis.Javascript/JavascriptInterop.cpp index 98cf5fa..1b354be 100644 --- a/Source/Noesis.Javascript/JavascriptInterop.cpp +++ b/Source/Noesis.Javascript/JavascriptInterop.cpp @@ -279,7 +279,7 @@ JavascriptInterop::WrapObject(System::Object^ iObject) v8::Isolate *isolate = JavascriptContext::GetCurrentIsolate(); Local instanceTemplate = templ->InstanceTemplate(); Local object = instanceTemplate->NewInstance(isolate->GetCurrentContext()).ToLocalChecked(); - JavascriptExternal* external = context->WrapObject(iObject); + JavascriptExternal* external = new JavascriptExternal(iObject, true); object->SetInternalField(0, External::New(isolate, external)); // So we're notified when this object is no longer needed.