Skip to content

Commit c6bed0a

Browse files
spahnkespahnke
authored
Reduce memory inefficiencies and leaks (Merge pull request #95)
2 parents 9ac3b88 + 5412f8d commit c6bed0a

File tree

8 files changed

+135
-126
lines changed

8 files changed

+135
-126
lines changed

‎Source/Noesis.Javascript/JavascriptContext.cpp

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -159,8 +159,8 @@ JavascriptContext::JavascriptContext()
159159
isolate->SetFatalErrorHandler(FatalErrorCallback);
160160

161161
mExternals = gcnew System::Collections::Generic::Dictionary<System::Object ^, WrappedJavascriptExternal>();
162+
mMethods = gcnew System::Collections::Generic::Dictionary<System::String ^, WrappedMethod>();
162163
mTypeToConstructorMapping = gcnew System::Collections::Generic::Dictionary<System::Type ^, System::IntPtr>();
163-
mFunctions = gcnew System::Collections::Generic::List<System::WeakReference ^>();
164164
HandleScope scope(isolate);
165165
mContext = new Persistent<Context>(isolate, Context::New(isolate));
166166
terminateRuns = false;
@@ -175,18 +175,19 @@ JavascriptContext::~JavascriptContext()
175175
v8::Isolate::Scope isolate_scope(isolate);
176176
for each (WrappedJavascriptExternal wrapped in mExternals->Values)
177177
delete wrapped.Pointer;
178-
for each (System::WeakReference^ f in mFunctions) {
179-
JavascriptFunction ^function = safe_cast<JavascriptFunction ^>(f->Target);
180-
if (function != nullptr)
181-
delete function;
178+
for each (WrappedMethod wrapped in mMethods->Values)
179+
{
180+
wrapped.Pointer->Reset();
181+
delete wrapped.Pointer;
182182
}
183183
for each (System::IntPtr p in mTypeToConstructorMapping->Values) {
184184
delete (void *)p;
185185
}
186186
delete mContext;
187+
mContext = nullptr;
187188
delete mExternals;
189+
delete mMethods;
188190
delete mTypeToConstructorMapping;
189-
delete mFunctions;
190191
}
191192
if (isolate != NULL)
192193
isolate->Dispose();
@@ -525,16 +526,6 @@ JavascriptContext::GetObjectWrapperConstructorTemplate(System::Type ^type)
525526

526527
////////////////////////////////////////////////////////////////////////////////////////////////////
527528

528-
void
529-
JavascriptContext::RegisterFunction(System::Object^ f)
530-
{
531-
// Note that while we do store WeakReferences, we never clean up this hashtable,
532-
// so it will just grow and grow.
533-
mFunctions->Add(gcnew System::WeakReference(f));
534-
}
535-
536-
////////////////////////////////////////////////////////////////////////////////////////////////////
537-
538529
System::String^ JavascriptContext::V8Version::get()
539530
{
540531
return gcnew System::String(v8::V8::GetVersion());

‎Source/Noesis.Javascript/JavascriptContext.h

Lines changed: 15 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -66,16 +66,16 @@ public enum class SetParameterOptions : int
6666
public value struct WrappedMethod
6767
{
6868
private:
69-
System::IntPtr pointer;
69+
System::IntPtr pointer;
7070

7171
internal:
72-
WrappedMethod(Persistent<Function> *value)
73-
{
74-
System::IntPtr value_pointer(value);
72+
WrappedMethod(Persistent<Function> *value)
73+
{
74+
System::IntPtr value_pointer(value);
7575
pointer = value_pointer;
76-
}
76+
}
7777

78-
property Persistent<Function> *Pointer
78+
property Persistent<Function> *Pointer
7979
{
8080
Persistent<Function> *get()
8181
{
@@ -196,14 +196,20 @@ public ref class JavascriptContext: public System::IDisposable
196196

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

199-
void RegisterFunction(System::Object^ f);
200-
201199
static void FatalErrorCallbackMember(const char* location, const char* message);
202200

201+
inline bool IsDisposed() { return mContext == nullptr; }
202+
203203
////////////////////////////////////////////////////////////
204204
// Data members
205205
////////////////////////////////////////////////////////////
206-
206+
internal:
207+
// Stores every JavascriptExternal we create. This saves time if the same
208+
// objects are recreated frequently, and stops us building up a huge
209+
// collection of JavascriptExternal objects that won't be freed until
210+
// the context is destroyed.
211+
System::Collections::Generic::Dictionary<System::Object^, WrappedJavascriptExternal>^ mExternals;
212+
System::Collections::Generic::Dictionary<System::String^, WrappedMethod>^ mMethods;
207213
protected:
208214
// By entering an isolate before using a context, we can have multiple
209215
// contexts used simultaneously in different threads.
@@ -212,22 +218,12 @@ public ref class JavascriptContext: public System::IDisposable
212218
// v8 context required to be active for all v8 operations.
213219
Persistent<Context>* mContext;
214220

215-
// Stores every JavascriptExternal we create. This saves time if the same
216-
// objects are recreated frequently, and stops us building up a huge
217-
// collection of JavascriptExternal objects that won't be freed until
218-
// the context is destroyed.
219-
System::Collections::Generic::Dictionary<System::Object ^, WrappedJavascriptExternal> ^mExternals;
220-
221221
// Maps types to their constructor function templates
222222
// The mapping will either be defined by the user calling `SetConstructor` or autogenerated if no
223223
// mapping was provided.
224224
// The `IntPtr` points to a `Persistent<FunctionTemplate>`.
225225
System::Collections::Generic::Dictionary<System::Type ^, System::IntPtr> ^mTypeToConstructorMapping;
226226

227-
// Stores every JavascriptFunction we create. Ensures we dispose of them
228-
// all.
229-
System::Collections::Generic::List<System::WeakReference ^> ^mFunctions;
230-
231227
// See comment for TerminateExecution().
232228
bool terminateRuns;
233229

‎Source/Noesis.Javascript/JavascriptExternal.cpp

Lines changed: 71 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -52,14 +52,38 @@ JavascriptExternal::JavascriptExternal(System::Object^ iObject)
5252
{
5353
mObjectHandle = System::Runtime::InteropServices::GCHandle::Alloc(iObject);
5454
mOptions = SetParameterOptions::None;
55-
mMethods = gcnew System::Collections::Generic::Dictionary<System::String ^, WrappedMethod>();
5655
}
5756

5857
////////////////////////////////////////////////////////////////////////////////////////////////////
5958

6059
JavascriptExternal::~JavascriptExternal()
6160
{
62-
mObjectHandle.Free();
61+
mObjectHandle.Free();
62+
if (!mPersistent.IsEmpty()) {
63+
mPersistent.ClearWeak<void>();
64+
mPersistent.Reset();
65+
}
66+
67+
}
68+
69+
////////////////////////////////////////////////////////////////////////////////////////////////////
70+
71+
void GCCallback(const WeakCallbackInfo<JavascriptExternal>& data)
72+
{
73+
auto context = JavascriptContext::GetCurrent();
74+
auto external = data.GetParameter();
75+
auto object = external->GetObject();
76+
if (context->mExternals->ContainsKey(object))
77+
context->mExternals->Remove(object);
78+
delete external;
79+
}
80+
81+
void
82+
JavascriptExternal::Wrap(Isolate* isolate, Local<Object> object)
83+
{
84+
object->SetInternalField(0, External::New(isolate, this));
85+
mPersistent.Reset(isolate, object);
86+
mPersistent.SetWeak(this, &GCCallback, WeakCallbackType::kParameter);
6387
}
6488

6589
////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -75,40 +99,28 @@ JavascriptExternal::GetObject()
7599
Local<Function>
76100
JavascriptExternal::GetMethod(wstring iName)
77101
{
78-
v8::Isolate *isolate = JavascriptContext::GetCurrentIsolate();
79-
System::Collections::Generic::Dictionary<System::String ^, WrappedMethod> ^methods = mMethods;
80-
System::String^ memberName = gcnew System::String(iName.c_str());
81-
WrappedMethod method;
82-
if (methods->TryGetValue(memberName, method))
83-
return Local<Function>::New(isolate, *(method.Pointer));
84-
else
85-
{
86-
System::Object^ self = mObjectHandle.Target;
87-
System::Type^ type = self->GetType();
88-
System::String^ memberName = gcnew System::String(iName.c_str());
89-
cli::array<System::Object^>^ objectInfo = gcnew cli::array<System::Object^>(2);
90-
objectInfo->SetValue(self,0);
91-
objectInfo->SetValue(memberName,1);
92-
93-
// Verification if it a method
94-
cli::array<MemberInfo^>^ members = type->GetMember(memberName);
95-
if (members->Length > 0 && members[0]->MemberType == MemberTypes::Method)
96-
{
97-
JavascriptContext^ context = JavascriptContext::GetCurrent();
98-
Local<External> external = External::New(isolate, context->WrapObject(objectInfo));
99-
Local<FunctionTemplate> functionTemplate = FunctionTemplate::New(isolate, JavascriptInterop::Invoker, external);
100-
Local<Function> function = functionTemplate->GetFunction(isolate->GetCurrentContext()).ToLocalChecked();
102+
auto context = JavascriptContext::GetCurrent();
103+
auto isolate = JavascriptContext::GetCurrentIsolate();
101104

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

106-
return function;
107-
}
108-
}
109+
if (context->mMethods->ContainsKey(uniqueMethodName))
110+
return Local<Function>::New(isolate, *context->mMethods[uniqueMethodName].Pointer);
111+
112+
// Verification if it a method
113+
auto members = type->GetMember(memberName);
114+
if (members->Length > 0 && members[0]->MemberType == MemberTypes::Method)
115+
{
116+
auto functionTemplate = FunctionTemplate::New(isolate, JavascriptInterop::Invoker, JavascriptInterop::ConvertToV8(memberName));
117+
auto function = functionTemplate->GetFunction(isolate->GetCurrentContext()).ToLocalChecked();
118+
context->mMethods[uniqueMethodName] = WrappedMethod(new Persistent<Function>(isolate, function));
119+
return function;
120+
}
109121

110122
// Wasn't an method
111-
return Local<Function>();
123+
return Local<Function>();
112124
}
113125

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

334346
Local<Function> JavascriptExternal::GetIterator()
335347
{
348+
auto context = JavascriptContext::GetCurrent();
349+
auto type = mObjectHandle.Target->GetType();
350+
auto uniqueMethodName = type->AssemblyQualifiedName + L".$$Iterator";
351+
336352
auto isolate = JavascriptContext::GetCurrentIsolate();
337-
if (mIterator != nullptr)
338-
return Local<Function>::New(isolate, *mIterator);
353+
if (context->mMethods->ContainsKey(uniqueMethodName))
354+
return Local<Function>::New(isolate, *context->mMethods[uniqueMethodName].Pointer);
339355

340-
auto self = mObjectHandle.Target;
341-
auto context = JavascriptContext::GetCurrent();
342-
auto external = External::New(isolate, context->WrapObject(self));
343-
auto functionTemplate = FunctionTemplate::New(isolate, JavascriptExternal::IteratorCallback, external);
356+
auto functionTemplate = FunctionTemplate::New(isolate, JavascriptExternal::IteratorCallback);
344357
auto function = functionTemplate->GetFunction(isolate->GetCurrentContext()).ToLocalChecked();
345-
mIterator = std::make_unique<Persistent<Function>>(isolate, function);
358+
context->mMethods[uniqueMethodName] = WrappedMethod(new Persistent<Function>(isolate, function));
346359
return function;
347360
}
348361

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

357366
auto iterator = ObjectTemplate::New(isolate);
358-
auto functionTemplate = FunctionTemplate::New(isolate, JavascriptExternal::IteratorNextCallback, external);
367+
iterator->SetInternalFieldCount(1);
368+
auto functionTemplate = FunctionTemplate::New(isolate, JavascriptExternal::IteratorNextCallback);
359369
iterator->Set(String::NewFromUtf8(isolate, "next").ToLocalChecked(), functionTemplate);
360-
iArgs.GetReturnValue().Set(iterator->NewInstance(isolate->GetCurrentContext()).ToLocalChecked());
370+
auto iteratorInstance = iterator->NewInstance(isolate->GetCurrentContext()).ToLocalChecked();
371+
372+
auto internalField = Local<External>::Cast(iArgs.Holder()->GetInternalField(0));
373+
auto external = (JavascriptExternal*)internalField->Value();
374+
auto enumerable = (System::Collections::IEnumerable^)external->GetObject();
375+
auto enumerator = enumerable->GetEnumerator();
376+
377+
auto context = JavascriptContext::GetCurrent();
378+
auto enumeratorExternal = context->WrapObject(enumerator);
379+
enumeratorExternal->Wrap(isolate, iteratorInstance);
380+
381+
iArgs.GetReturnValue().Set(iteratorInstance);
361382
}
362383

363384
void JavascriptExternal::IteratorNextCallback(const v8::FunctionCallbackInfo<Value>& iArgs)
364385
{
365386
auto isolate = iArgs.GetIsolate();
366-
auto enumerator = (System::Collections::IEnumerator^) JavascriptInterop::UnwrapObject(Local<External>::Cast(iArgs.Data()));
387+
388+
auto internalField = Local<External>::Cast(iArgs.Holder()->GetInternalField(0));
389+
auto external = (JavascriptExternal*)internalField->Value();
390+
auto enumerator = (System::Collections::IEnumerator^) external->GetObject();
367391
auto done = !enumerator->MoveNext();
368392

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

379403
} } // namespace Noesis::Javascript
380404

381-
////////////////////////////////////////////////////////////////////////////////////////////////////
405+
////////////////////////////////////////////////////////////////////////////////////////////////////

‎Source/Noesis.Javascript/JavascriptExternal.h

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@
3131
////////////////////////////////////////////////////////////////////////////////////////////////////
3232

3333
#include <v8.h>
34-
#include <map>
3534
#include <gcroot.h>
3635
#include "JavascriptContext.h"
3736

@@ -87,9 +86,13 @@ class JavascriptExternal
8786

8887
Local<Function> GetIterator();
8988

89+
void Wrap(Isolate* isolate, Local<Object> object);
90+
9091
////////////////////////////////////////////////////////////
9192
// Data members
9293
////////////////////////////////////////////////////////////
94+
public:
95+
Persistent<Object> mPersistent;
9396
private:
9497

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

99102
SetParameterOptions mOptions;
100103

101-
// Owned by JavascriptContext.
102-
gcroot<System::Collections::Generic::Dictionary<System::String ^, WrappedMethod> ^> mMethods;
103-
104-
std::unique_ptr<Persistent<Function>> mIterator;
105104
static void IteratorCallback(const v8::FunctionCallbackInfo<Value>& iArgs);
106105
static void IteratorNextCallback(const v8::FunctionCallbackInfo<Value>& iArgs);
107106
};

0 commit comments

Comments
 (0)