Skip to content

Clean up external object references when they are garbage collected, … #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Clean up external object references when they are garbage collected, … #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
612f08e
Clean up external object references when they are garbage collected, …
oliverbock Aug 3, 2020
f5de7b8
Cache dictionary of methods with JavaScriptExternal to make Getter() …
oliverbock Aug 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 14 additions & 16 deletions Source/Noesis.Javascript/JavascriptContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,9 @@ JavascriptContext::JavascriptContext()

isolate->SetFatalErrorHandler(FatalErrorCallback);

mExternals = gcnew System::Collections::Generic::Dictionary<System::Object ^, WrappedJavascriptExternal>();
mTypeToConstructorMapping = gcnew System::Collections::Generic::Dictionary<System::Type ^, System::IntPtr>();
mTypeToMethods = gcnew System::Collections::Generic::Dictionary<System::Type^, System::Collections::Generic::Dictionary<System::String^, WrappedMethod>^>();

mFunctions = gcnew System::Collections::Generic::List<System::WeakReference ^>();
HandleScope scope(isolate);
mContext = new Persistent<Context>(isolate, Context::New(isolate));
Expand All @@ -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<JavascriptFunction ^>(f->Target);
if (function != nullptr)
Expand All @@ -197,8 +196,8 @@ JavascriptContext::~JavascriptContext()
delete (void *)p;
}
delete mContext;
delete mExternals;
delete mTypeToConstructorMapping;
delete mTypeToMethods;
delete mFunctions;
}
if (isolate != NULL)
Expand Down Expand Up @@ -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<FunctionTemplate>(isolate, functionTemplate));
mTypeToMethods[associatedType] = gcnew System::Collections::Generic::Dictionary<System::String^, WrappedMethod>();
Local<Context>::New(isolate, *mContext)->Global()->Set(context, className, functionTemplate->GetFunction(context).ToLocalChecked());
}

Expand Down Expand Up @@ -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, false);
}

////////////////////////////////////////////////////////////////////////////////////////////////////
Expand All @@ -529,6 +518,7 @@ JavascriptContext::GetObjectWrapperConstructorTemplate(System::Type ^type)
Local<FunctionTemplate> constructor = FunctionTemplate::New(GetCurrentIsolate());
JavascriptInterop::InitObjectWrapperTemplate(constructor->InstanceTemplate());
mTypeToConstructorMapping[type] = System::IntPtr(new Persistent<FunctionTemplate>(isolate, constructor));
mTypeToMethods[type] = gcnew System::Collections::Generic::Dictionary<System::String^, WrappedMethod>();
return constructor;
}
Persistent<FunctionTemplate> *constructor = (Persistent<FunctionTemplate> *)(void *)ptrToConstructor;
Expand All @@ -537,6 +527,14 @@ JavascriptContext::GetObjectWrapperConstructorTemplate(System::Type ^type)

////////////////////////////////////////////////////////////////////////////////////////////////////

System::Collections::Generic::Dictionary<System::String^, WrappedMethod>^
JavascriptContext::MethodsForType(System::Type^ type)
{
return mTypeToMethods[type];
}

////////////////////////////////////////////////////////////////////////////////////////////////////

void
JavascriptContext::RegisterFunction(System::Object^ f)
{
Expand Down
14 changes: 7 additions & 7 deletions Source/Noesis.Javascript/JavascriptContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,9 @@ public ref class JavascriptContext: public System::IDisposable

Local<FunctionTemplate> GetObjectWrapperConstructorTemplate(System::Type ^type);

void RegisterFunction(System::Object^ f);
System::Collections::Generic::Dictionary<System::String^, WrappedMethod>^ MethodsForType(System::Type^ type);

void RegisterFunction(System::Object^ f);

static void FatalErrorCallbackMember(const char* location, const char* message);

Expand All @@ -212,18 +214,16 @@ public ref class JavascriptContext: public System::IDisposable
// v8 context required to be active for all v8 operations.
Persistent<Context>* 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<System::Object ^, WrappedJavascriptExternal> ^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<FunctionTemplate>`.
System::Collections::Generic::Dictionary<System::Type ^, System::IntPtr> ^mTypeToConstructorMapping;

// For each distinct CLR type we've wrapped, this accumulates functions
// constructed to access its methods/properties.
System::Collections::Generic::Dictionary<System::Type ^, System::Collections::Generic::Dictionary<System::String^, WrappedMethod>^>^ mTypeToMethods;

// Stores every JavascriptFunction we create. Ensures we dispose of them
// all.
System::Collections::Generic::List<System::WeakReference ^> ^mFunctions;
Expand Down
22 changes: 12 additions & 10 deletions Source/Noesis.Javascript/JavascriptExternal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +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;
mMethods = gcnew System::Collections::Generic::Dictionary<System::String ^, WrappedMethod>();
System::Type^ type = iObject->GetType();
if (needs_methods)
mMethods = JavascriptContext::GetCurrent()->MethodsForType(type);
}

////////////////////////////////////////////////////////////////////////////////////////////////////
Expand All @@ -76,16 +78,16 @@ Local<Function>
JavascriptExternal::GetMethod(wstring iName)
{
v8::Isolate *isolate = JavascriptContext::GetCurrentIsolate();
System::Collections::Generic::Dictionary<System::String ^, WrappedMethod> ^methods = mMethods;
System::String^ memberName = gcnew System::String(iName.c_str());
System::Collections::Generic::Dictionary<System::String^, WrappedMethod>^ methods = mMethods;
System::String^ memberName = gcnew System::String(iName.c_str());
WrappedMethod method;
if (methods->TryGetValue(memberName, method))
return Local<Function>::New(isolate, *(method.Pointer));
else
{
System::Object^ self = mObjectHandle.Target;
System::Type^ type = self->GetType();
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<System::Object^>^ objectInfo = gcnew cli::array<System::Object^>(2);
objectInfo->SetValue(self,0);
objectInfo->SetValue(memberName,1);
Expand All @@ -94,14 +96,14 @@ JavascriptExternal::GetMethod(wstring iName)
cli::array<MemberInfo^>^ members = type->GetMember(memberName);
if (members->Length > 0 && members[0]->MemberType == MemberTypes::Method)
{
JavascriptContext^ context = JavascriptContext::GetCurrent();
Local<External> external = External::New(isolate, context->WrapObject(objectInfo));
JavascriptContext^ context = JavascriptContext::GetCurrent();
Local<External> external = External::New(isolate, context->WrapObject(objectInfo));
Local<FunctionTemplate> functionTemplate = FunctionTemplate::New(isolate, JavascriptInterop::Invoker, external);
Local<Function> function = functionTemplate->GetFunction(isolate->GetCurrentContext()).ToLocalChecked();

Persistent<Function> *function_ptr = new Persistent<Function>(isolate, function);
WrappedMethod wrapped(function_ptr);
methods[memberName] = wrapped;
methods[memberName] = wrapped;

return function;
}
Expand Down
6 changes: 3 additions & 3 deletions Source/Noesis.Javascript/JavascriptExternal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class JavascriptExternal
////////////////////////////////////////////////////////////
public:

JavascriptExternal(System::Object^ iObject);
JavascriptExternal(System::Object^ iObject, bool needs_methods);

~JavascriptExternal();

Expand Down Expand Up @@ -98,8 +98,8 @@ class JavascriptExternal

SetParameterOptions mOptions;

// Owned by JavascriptContext.
gcroot<System::Collections::Generic::Dictionary<System::String ^, WrappedMethod> ^> mMethods;
// Owned by JavascriptContext. Only set when the object is with `JavascriptInterop::Getter()`
gcroot<System::Collections::Generic::Dictionary<System::String^, WrappedMethod>^> mMethods;

std::unique_ptr<Persistent<Function>> mIterator;
static void IteratorCallback(const v8::FunctionCallbackInfo<Value>& iArgs);
Expand Down
21 changes: 17 additions & 4 deletions Source/Noesis.Javascript/JavascriptInterop.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,19 +259,32 @@ JavascriptInterop::ConvertToV8(System::Object^ iObject)

////////////////////////////////////////////////////////////////////////////////////////////////////

// TODO: should return Local<External>
// a sort-of destructor in v8 land
void JavascriptExternalMakeWeak(const v8::WeakCallbackInfo<JavascriptExternal>& data) {
v8::Isolate* isolate = JavascriptContext::GetCurrentIsolate();
JavascriptExternal *external = data.GetParameter();
delete external;
}

// TO DO: should return Local<External>
Local<Object>
JavascriptInterop::WrapObject(System::Object^ iObject)
{
JavascriptContext^ context = JavascriptContext::GetCurrent();

if (context != nullptr)
{
Local<FunctionTemplate> templ = context->GetObjectWrapperConstructorTemplate(iObject->GetType());
System::Type ^object_type = iObject->GetType();
Local<FunctionTemplate> templ = context->GetObjectWrapperConstructorTemplate(object_type);
v8::Isolate *isolate = JavascriptContext::GetCurrentIsolate();
Local<ObjectTemplate> instanceTemplate = templ->InstanceTemplate();
Local<Object> object = instanceTemplate->NewInstance(isolate->GetCurrentContext()).ToLocalChecked();
object->SetInternalField(0, External::New(isolate, 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.
v8::Persistent<v8::Object, v8::CopyablePersistentTraits<v8::Object>> pobj(isolate, object);
pobj.SetWeak<JavascriptExternal>(external, JavascriptExternalMakeWeak, v8::WeakCallbackType::kParameter);

return object;
}
Expand All @@ -281,7 +294,7 @@ JavascriptInterop::WrapObject(System::Object^ iObject)

////////////////////////////////////////////////////////////////////////////////////////////////////

// TODO: should use Local<External> iExternal
// TO DO: should use Local<External> iExternal
System::Object^
JavascriptInterop::UnwrapObject(Local<Value> iValue)
{
Expand Down