Skip to content
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

Reduce memory inefficiencies and leaks #95

Merged
merged 7 commits into from
Apr 26, 2021
+135 −126
Merged

Reduce memory inefficiencies and leaks #95

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
85a9b79
Set up a callback function that runs when a JS object becomes weak th…
spahnke Mar 29, 2021
e7c1f5a
Cache WrappedMethod at context level and get rid of object[] allocati…
spahnke Apr 1, 2021
84aa13a
Reduce wrapping in iterator callbacks (fixes iterators returned by me…
spahnke Apr 6, 2021
6f50436
Use existing method cache for iterator functions as well
spahnke Apr 6, 2021
da8cc41
Prefer a weak reference to the JavascriptContext in JavascriptFunctio…
spahnke Apr 7, 2021
c710a90
Remove unused method
spahnke Apr 21, 2021
5412f8d
Invert if statement
spahnke Apr 26, 2021
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
23 changes: 7 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 @@ -159,8 +159,8 @@ JavascriptContext::JavascriptContext()
isolate->SetFatalErrorHandler(FatalErrorCallback);

mExternals = gcnew System::Collections::Generic::Dictionary<System::Object ^, WrappedJavascriptExternal>();
mMethods = gcnew System::Collections::Generic::Dictionary<System::String ^, WrappedMethod>();
mTypeToConstructorMapping = gcnew System::Collections::Generic::Dictionary<System::Type ^, System::IntPtr>();
mFunctions = gcnew System::Collections::Generic::List<System::WeakReference ^>();
HandleScope scope(isolate);
mContext = new Persistent<Context>(isolate, Context::New(isolate));
terminateRuns = false;
Expand All @@ -175,18 +175,19 @@ JavascriptContext::~JavascriptContext()
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)
delete function;
for each (WrappedMethod wrapped in mMethods->Values)
{
wrapped.Pointer->Reset();
delete wrapped.Pointer;
}
for each (System::IntPtr p in mTypeToConstructorMapping->Values) {
delete (void *)p;
}
delete mContext;
mContext = nullptr;
delete mExternals;
delete mMethods;
delete mTypeToConstructorMapping;
delete mFunctions;
}
if (isolate != NULL)
isolate->Dispose();
Expand Down Expand Up @@ -525,16 +526,6 @@ JavascriptContext::GetObjectWrapperConstructorTemplate(System::Type ^type)

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

void
JavascriptContext::RegisterFunction(System::Object^ f)
{
// Note that while we do store WeakReferences, we never clean up this hashtable,
// so it will just grow and grow.
mFunctions->Add(gcnew System::WeakReference(f));
}

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

System::String^ JavascriptContext::V8Version::get()
{
return gcnew System::String(v8::V8::GetVersion());
Expand Down
34 changes: 15 additions & 19 deletions Source/Noesis.Javascript/JavascriptContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,16 +66,16 @@ public enum class SetParameterOptions : int
public value struct WrappedMethod
{
private:
System::IntPtr pointer;
System::IntPtr pointer;

internal:
WrappedMethod(Persistent<Function> *value)
{
System::IntPtr value_pointer(value);
WrappedMethod(Persistent<Function> *value)
{
System::IntPtr value_pointer(value);
pointer = value_pointer;
}
}

property Persistent<Function> *Pointer
property Persistent<Function> *Pointer
{
Persistent<Function> *get()
{
Expand Down Expand Up @@ -196,14 +196,20 @@ public ref class JavascriptContext: public System::IDisposable

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

void RegisterFunction(System::Object^ f);

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

inline bool IsDisposed() { return mContext == nullptr; }

////////////////////////////////////////////////////////////
// Data members
////////////////////////////////////////////////////////////

internal:
// 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;
System::Collections::Generic::Dictionary<System::String^, WrappedMethod>^ mMethods;
protected:
// By entering an isolate before using a context, we can have multiple
// contexts used simultaneously in different threads.
Expand All @@ -212,22 +218,12 @@ 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;

// Stores every JavascriptFunction we create. Ensures we dispose of them
// all.
System::Collections::Generic::List<System::WeakReference ^> ^mFunctions;

// See comment for TerminateExecution().
bool terminateRuns;

Expand Down
118 changes: 71 additions & 47 deletions Source/Noesis.Javascript/JavascriptExternal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,38 @@ JavascriptExternal::JavascriptExternal(System::Object^ iObject)
{
mObjectHandle = System::Runtime::InteropServices::GCHandle::Alloc(iObject);
mOptions = SetParameterOptions::None;
mMethods = gcnew System::Collections::Generic::Dictionary<System::String ^, WrappedMethod>();
}

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

JavascriptExternal::~JavascriptExternal()
{
mObjectHandle.Free();
mObjectHandle.Free();
if (!mPersistent.IsEmpty()) {
mPersistent.ClearWeak<void>();
mPersistent.Reset();
}

}

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

void GCCallback(const WeakCallbackInfo<JavascriptExternal>& data)
{
auto context = JavascriptContext::GetCurrent();
auto external = data.GetParameter();
auto object = external->GetObject();
if (context->mExternals->ContainsKey(object))
context->mExternals->Remove(object);
delete external;
}

void
JavascriptExternal::Wrap(Isolate* isolate, Local<Object> object)
{
object->SetInternalField(0, External::New(isolate, this));
mPersistent.Reset(isolate, object);
mPersistent.SetWeak(this, &GCCallback, WeakCallbackType::kParameter);
}

////////////////////////////////////////////////////////////////////////////////////////////////////
Expand All @@ -75,40 +99,28 @@ JavascriptExternal::GetObject()
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());
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());
cli::array<System::Object^>^ objectInfo = gcnew cli::array<System::Object^>(2);
objectInfo->SetValue(self,0);
objectInfo->SetValue(memberName,1);

// Verification if it a method
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));
Local<FunctionTemplate> functionTemplate = FunctionTemplate::New(isolate, JavascriptInterop::Invoker, external);
Local<Function> function = functionTemplate->GetFunction(isolate->GetCurrentContext()).ToLocalChecked();
auto context = JavascriptContext::GetCurrent();
auto isolate = JavascriptContext::GetCurrentIsolate();

Persistent<Function> *function_ptr = new Persistent<Function>(isolate, function);
WrappedMethod wrapped(function_ptr);
methods[memberName] = wrapped;
auto type = mObjectHandle.Target->GetType();
auto memberName = gcnew System::String(iName.c_str());
auto uniqueMethodName = type->AssemblyQualifiedName + L"." + memberName;

return function;
}
}
if (context->mMethods->ContainsKey(uniqueMethodName))
return Local<Function>::New(isolate, *context->mMethods[uniqueMethodName].Pointer);

// Verification if it a method
auto members = type->GetMember(memberName);
if (members->Length > 0 && members[0]->MemberType == MemberTypes::Method)
{
auto functionTemplate = FunctionTemplate::New(isolate, JavascriptInterop::Invoker, JavascriptInterop::ConvertToV8(memberName));
auto function = functionTemplate->GetFunction(isolate->GetCurrentContext()).ToLocalChecked();
context->mMethods[uniqueMethodName] = WrappedMethod(new Persistent<Function>(isolate, function));
return function;
}

// Wasn't an method
return Local<Function>();
return Local<Function>();
}

////////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -333,37 +345,49 @@ JavascriptExternal::SetProperty(uint32_t iIndex, Local<Value> iValue)

Local<Function> JavascriptExternal::GetIterator()
{
auto context = JavascriptContext::GetCurrent();
auto type = mObjectHandle.Target->GetType();
auto uniqueMethodName = type->AssemblyQualifiedName + L".$$Iterator";

auto isolate = JavascriptContext::GetCurrentIsolate();
if (mIterator != nullptr)
return Local<Function>::New(isolate, *mIterator);
if (context->mMethods->ContainsKey(uniqueMethodName))
return Local<Function>::New(isolate, *context->mMethods[uniqueMethodName].Pointer);

auto self = mObjectHandle.Target;
auto context = JavascriptContext::GetCurrent();
auto external = External::New(isolate, context->WrapObject(self));
auto functionTemplate = FunctionTemplate::New(isolate, JavascriptExternal::IteratorCallback, external);
auto functionTemplate = FunctionTemplate::New(isolate, JavascriptExternal::IteratorCallback);
auto function = functionTemplate->GetFunction(isolate->GetCurrentContext()).ToLocalChecked();
mIterator = std::make_unique<Persistent<Function>>(isolate, function);
context->mMethods[uniqueMethodName] = WrappedMethod(new Persistent<Function>(isolate, function));
return function;
}

void JavascriptExternal::IteratorCallback(const v8::FunctionCallbackInfo<Value>& iArgs)
{
auto isolate = iArgs.GetIsolate();
auto enumerable = (System::Collections::IEnumerable^) JavascriptInterop::UnwrapObject(Local<External>::Cast(iArgs.Data()));
auto enumerator = enumerable->GetEnumerator();
auto context = JavascriptContext::GetCurrent();
auto external = External::New(isolate, context->WrapObject(enumerator));

auto iterator = ObjectTemplate::New(isolate);
auto functionTemplate = FunctionTemplate::New(isolate, JavascriptExternal::IteratorNextCallback, external);
iterator->SetInternalFieldCount(1);
auto functionTemplate = FunctionTemplate::New(isolate, JavascriptExternal::IteratorNextCallback);
iterator->Set(String::NewFromUtf8(isolate, "next").ToLocalChecked(), functionTemplate);
iArgs.GetReturnValue().Set(iterator->NewInstance(isolate->GetCurrentContext()).ToLocalChecked());
auto iteratorInstance = iterator->NewInstance(isolate->GetCurrentContext()).ToLocalChecked();

auto internalField = Local<External>::Cast(iArgs.Holder()->GetInternalField(0));
auto external = (JavascriptExternal*)internalField->Value();
auto enumerable = (System::Collections::IEnumerable^)external->GetObject();
auto enumerator = enumerable->GetEnumerator();

auto context = JavascriptContext::GetCurrent();
auto enumeratorExternal = context->WrapObject(enumerator);
enumeratorExternal->Wrap(isolate, iteratorInstance);

iArgs.GetReturnValue().Set(iteratorInstance);
}

void JavascriptExternal::IteratorNextCallback(const v8::FunctionCallbackInfo<Value>& iArgs)
{
auto isolate = iArgs.GetIsolate();
auto enumerator = (System::Collections::IEnumerator^) JavascriptInterop::UnwrapObject(Local<External>::Cast(iArgs.Data()));

auto internalField = Local<External>::Cast(iArgs.Holder()->GetInternalField(0));
auto external = (JavascriptExternal*)internalField->Value();
auto enumerator = (System::Collections::IEnumerator^) external->GetObject();
auto done = !enumerator->MoveNext();

auto resultTemplate = ObjectTemplate::New(isolate);
Expand All @@ -378,4 +402,4 @@ void JavascriptExternal::IteratorNextCallback(const v8::FunctionCallbackInfo<Val

} } // namespace Noesis::Javascript

////////////////////////////////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////////////////////////////////
9 changes: 4 additions & 5 deletions Source/Noesis.Javascript/JavascriptExternal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
////////////////////////////////////////////////////////////////////////////////////////////////////

#include <v8.h>
#include <map>
#include <gcroot.h>
#include "JavascriptContext.h"

Expand Down Expand Up @@ -87,9 +86,13 @@ class JavascriptExternal

Local<Function> GetIterator();

void Wrap(Isolate* isolate, Local<Object> object);

////////////////////////////////////////////////////////////
// Data members
////////////////////////////////////////////////////////////
public:
Persistent<Object> mPersistent;
private:

// Handle to the .Net object being wrapped. It takes this
Expand All @@ -98,10 +101,6 @@ class JavascriptExternal

SetParameterOptions mOptions;

// Owned by JavascriptContext.
gcroot<System::Collections::Generic::Dictionary<System::String ^, WrappedMethod> ^> mMethods;

std::unique_ptr<Persistent<Function>> mIterator;
static void IteratorCallback(const v8::FunctionCallbackInfo<Value>& iArgs);
static void IteratorNextCallback(const v8::FunctionCallbackInfo<Value>& iArgs);
};
Expand Down
Loading