From 7d843f112b94b424fc74e65eeb6b2d1626d47136 Mon Sep 17 00:00:00 2001 From: Kyle Maxwell Date: Thu, 19 Jan 2023 13:17:30 -0800 Subject: [PATCH] v8.Value becomes manually releaseable (#361) * values are manually releaseable Co-authored-by: Kyle Maxwell --- CHANGELOG.md | 3 ++ context.go | 6 +++ context_test.go | 4 +- function_template.go | 7 ++++ function_template_test.go | 39 ++++++++++++++++++++ v8go.cc | 78 ++++++++++++++++++++++++++++++--------- v8go.h | 2 + value.go | 5 +++ 8 files changed, 124 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab1b80bd..8cd8d1cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added +- Added support for Value.release() and FunctionCallbackInfo.release(). This is useful when using v8go in a long-running context. + ### Fixed - Use string length to ensure null character-containing strings in Go/JS are not terminated early. - Object.Set with an empty key string is now supported diff --git a/context.go b/context.go index 69a81c3e..1176d45f 100644 --- a/context.go +++ b/context.go @@ -81,6 +81,12 @@ func (c *Context) Isolate() *Isolate { return c.iso } +func (c *Context) RetainedValueCount() int { + ctxMutex.Lock() + defer ctxMutex.Unlock() + return int(C.ContextRetainedValueCount(c.ptr)) +} + // RunScript executes the source JavaScript; origin (a.k.a. filename) provides a // reference for the script and used in the stack trace if there is an error. // error will be of type `JSError` if not nil. diff --git a/context_test.go b/context_test.go index 10dad5d8..503a463a 100644 --- a/context_test.go +++ b/context_test.go @@ -104,8 +104,8 @@ func TestMemoryLeak(t *testing.T) { for i := 0; i < 6000; i++ { ctx := v8.NewContext(iso) - obj := ctx.Global() - _ = obj.String() + _ = ctx.Global() + // _ = obj.String() _, _ = ctx.RunScript("2", "") ctx.Close() } diff --git a/function_template.go b/function_template.go index 5fad3808..66f1b8ce 100644 --- a/function_template.go +++ b/function_template.go @@ -37,6 +37,13 @@ func (i *FunctionCallbackInfo) Args() []*Value { return i.args } +func (i *FunctionCallbackInfo) Release() { + for _, arg := range i.args { + arg.Release() + } + i.this.Release() +} + // FunctionTemplate is used to create functions at runtime. // There can only be one function created from a FunctionTemplate in a context. // The lifetime of the created function is equal to the lifetime of the context. diff --git a/function_template_test.go b/function_template_test.go index de2dd382..7bbe1bfd 100644 --- a/function_template_test.go +++ b/function_template_test.go @@ -48,6 +48,45 @@ func TestFunctionTemplate_panic_on_nil_callback(t *testing.T) { defer iso.Dispose() v8.NewFunctionTemplate(iso, nil) } +func TestFunctionTemplate_generates_values(t *testing.T) { + t.Parallel() + + iso := v8.NewIsolate() + defer iso.Dispose() + global := v8.NewObjectTemplate(iso) + printfn := v8.NewFunctionTemplate(iso, func(info *v8.FunctionCallbackInfo) *v8.Value { + fmt.Printf("%+v\n", info.Args()) + return nil + }) + global.Set("print", printfn, v8.ReadOnly) + ctx := v8.NewContext(iso, global) + defer ctx.Close() + ctx.RunScript("print('foo', 'bar', 0, 1)", "") + if ctx.RetainedValueCount() != 6 { + t.Errorf("expected 6 retained values, got: %d", ctx.RetainedValueCount()) + } +} + +func TestFunctionTemplate_releases_values(t *testing.T) { + t.Parallel() + + iso := v8.NewIsolate() + defer iso.Dispose() + global := v8.NewObjectTemplate(iso) + printfn := v8.NewFunctionTemplate(iso, func(info *v8.FunctionCallbackInfo) *v8.Value { + defer info.Release() + fmt.Printf("%+v\n", info.Args()) + return nil + }) + global.Set("print", printfn, v8.ReadOnly) + ctx := v8.NewContext(iso, global) + defer ctx.Close() + ctx.RunScript("print('foo', 'bar', 0, 1)", "") + // there is a constant factor associated with the global. + if ctx.RetainedValueCount() != 1 { + t.Errorf("expected 1 retained values, got: %d", ctx.RetainedValueCount()) + } +} func TestFunctionTemplateGetFunction(t *testing.T) { t.Parallel() diff --git a/v8go.cc b/v8go.cc index e55bf45f..b435db59 100644 --- a/v8go.cc +++ b/v8go.cc @@ -11,6 +11,7 @@ #include #include #include +#include #include #include "_cgo_export.h" @@ -26,12 +27,14 @@ const int ScriptCompilerEagerCompile = ScriptCompiler::kEagerCompile; struct m_ctx { Isolate* iso; - std::vector vals; + std::unordered_map vals; std::vector unboundScripts; Persistent ptr; + long nextValId; }; struct m_value { + long id; Isolate* iso; m_ctx* ctx; Persistent> ptr; @@ -106,20 +109,11 @@ static RtnError ExceptionError(TryCatch& try_catch, m_value* tracked_value(m_ctx* ctx, m_value* val) { // (rogchap) we track values against a context so that when the context is // closed (either manually or GC'd by Go) we can also release all the - // values associated with the context; previously the Go GC would not run - // quickly enough, as it has no understanding of the C memory allocation size. - // By doing so we hold pointers to all values that are created/returned to Go - // until the context is released; this is a compromise. - // Ideally we would be able to delete the value object and cancel the - // finalizer on the Go side, but we currently don't pass the Go ptr, but - // rather the C ptr. A potential future iteration would be to use an - // unordered_map, where we could do O(1) lookups for the value, but then know - // if the object has been finalized or not by being in the map or not. This - // would require some ref id for the value rather than passing the ptr between - // Go <--> C, which would be a significant change, as there are places where - // we get the context from the value, but if we then need the context to get - // the value, we would be in a circular bind. - ctx->vals.push_back(val); + // values associated with the context; + if (val->id == 0) { + val->id = ++ctx->nextValId; + ctx->vals[val->id] = val; + } return val; } @@ -273,6 +267,7 @@ ValuePtr IsolateThrowException(IsolatePtr iso, ValuePtr value) { Local throw_ret_val = iso->ThrowException(value->ptr.Get(iso)); m_value* new_val = new m_value; + new_val->id = 0; new_val->iso = iso; new_val->ctx = ctx; new_val->ptr = @@ -457,6 +452,7 @@ RtnValue ObjectTemplateNewInstance(TemplatePtr ptr, ContextPtr ctx) { } m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>(iso, obj); @@ -494,6 +490,7 @@ static void FunctionTemplateCallback(const FunctionCallbackInfo& info) { int callback_ref = info.Data().As()->Value(); m_value* _this = new m_value; + _this->id = 0; _this->iso = iso; _this->ctx = ctx; _this->ptr.Reset(iso, Persistent>( @@ -505,6 +502,7 @@ static void FunctionTemplateCallback(const FunctionCallbackInfo& info) { ValuePtr* args = thisAndArgs + 1; for (int i = 0; i < args_count; i++) { m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr.Reset( @@ -554,6 +552,7 @@ RtnValue FunctionTemplateGetFunction(TemplatePtr ptr, ContextPtr ctx) { } m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>(iso, fn); @@ -600,16 +599,22 @@ ContextPtr NewContext(IsolatePtr iso, return ctx; } +int ContextRetainedValueCount(ContextPtr ctx) { + return ctx->vals.size(); +} + void ContextFree(ContextPtr ctx) { if (ctx == nullptr) { return; } ctx->ptr.Reset(); - for (m_value* val : ctx->vals) { - val->ptr.Reset(); - delete val; + for (auto it = ctx->vals.begin(); it != ctx->vals.end(); ++it) { + auto value = it->second; + value->ptr.Reset(); + delete value; } + ctx->vals.clear(); for (m_unboundScript* us : ctx->unboundScripts) { us->ptr.Reset(); @@ -646,6 +651,7 @@ RtnValue RunScript(ContextPtr ctx, const char* source, const char* origin) { return rtn; } m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>(iso, result); @@ -695,6 +701,7 @@ RtnValue UnboundScriptRun(ContextPtr ctx, UnboundScriptPtr us_ptr) { return rtn; } m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>(iso, result); @@ -718,6 +725,7 @@ RtnValue JSONParse(ContextPtr ctx, const char* str) { return rtn; } m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>(iso, result); @@ -761,9 +769,20 @@ const char* JSONStringify(ContextPtr ctx, ValuePtr val) { return CopyString(json); } +void ValueRelease(ValuePtr ptr) { + if (ptr == nullptr) { + return; + } + + ptr->ctx->vals.erase(ptr->id); + ptr->ptr.Reset(); + delete ptr; +} + ValuePtr ContextGlobal(ContextPtr ctx) { LOCAL_CONTEXT(ctx); m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; @@ -795,6 +814,7 @@ ValuePtr ContextGlobal(ContextPtr ctx) { ValuePtr NewValueInteger(IsolatePtr iso, int32_t v) { ISOLATE_SCOPE_INTERNAL_CONTEXT(iso); m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>( @@ -805,6 +825,7 @@ ValuePtr NewValueInteger(IsolatePtr iso, int32_t v) { ValuePtr NewValueIntegerFromUnsigned(IsolatePtr iso, uint32_t v) { ISOLATE_SCOPE_INTERNAL_CONTEXT(iso); m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>( @@ -823,6 +844,7 @@ RtnValue NewValueString(IsolatePtr iso, const char* v, int v_length) { return rtn; } m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>(iso, str); @@ -833,6 +855,7 @@ RtnValue NewValueString(IsolatePtr iso, const char* v, int v_length) { ValuePtr NewValueNull(IsolatePtr iso) { ISOLATE_SCOPE_INTERNAL_CONTEXT(iso); m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>(iso, Null(iso)); @@ -842,6 +865,7 @@ ValuePtr NewValueNull(IsolatePtr iso) { ValuePtr NewValueUndefined(IsolatePtr iso) { ISOLATE_SCOPE_INTERNAL_CONTEXT(iso); m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = @@ -852,6 +876,7 @@ ValuePtr NewValueUndefined(IsolatePtr iso) { ValuePtr NewValueBoolean(IsolatePtr iso, int v) { ISOLATE_SCOPE_INTERNAL_CONTEXT(iso); m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>( @@ -862,6 +887,7 @@ ValuePtr NewValueBoolean(IsolatePtr iso, int v) { ValuePtr NewValueNumber(IsolatePtr iso, double v) { ISOLATE_SCOPE_INTERNAL_CONTEXT(iso); m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>( @@ -872,6 +898,7 @@ ValuePtr NewValueNumber(IsolatePtr iso, double v) { ValuePtr NewValueBigInt(IsolatePtr iso, int64_t v) { ISOLATE_SCOPE_INTERNAL_CONTEXT(iso); m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>( @@ -882,6 +909,7 @@ ValuePtr NewValueBigInt(IsolatePtr iso, int64_t v) { ValuePtr NewValueBigIntFromUnsigned(IsolatePtr iso, uint64_t v) { ISOLATE_SCOPE_INTERNAL_CONTEXT(iso); m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>( @@ -905,6 +933,7 @@ RtnValue NewValueBigIntFromWords(IsolatePtr iso, return rtn; } m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>(iso, bigint); @@ -1002,6 +1031,7 @@ RtnValue ValueToObject(ValuePtr ptr) { return rtn; } m_value* new_val = new m_value; + new_val->id = 0; new_val->iso = iso; new_val->ctx = ctx; new_val->ptr = Persistent>(iso, obj); @@ -1340,6 +1370,7 @@ RtnValue ObjectGet(ValuePtr ptr, const char* key) { return rtn; } m_value* new_val = new m_value; + new_val->id = 0; new_val->iso = iso; new_val->ctx = ctx; new_val->ptr = @@ -1359,6 +1390,7 @@ ValuePtr ObjectGetInternalField(ValuePtr ptr, int idx) { Local result = obj->GetInternalField(idx); m_value* new_val = new m_value; + new_val->id = 0; new_val->iso = iso; new_val->ctx = ctx; new_val->ptr = @@ -1377,6 +1409,7 @@ RtnValue ObjectGetIdx(ValuePtr ptr, uint32_t idx) { return rtn; } m_value* new_val = new m_value; + new_val->id = 0; new_val->iso = iso; new_val->ctx = ctx; new_val->ptr = @@ -1421,6 +1454,7 @@ RtnValue NewPromiseResolver(ContextPtr ctx) { return rtn; } m_value* val = new m_value; + val->id = 0; val->iso = iso; val->ctx = ctx; val->ptr = Persistent>(iso, resolver); @@ -1433,6 +1467,7 @@ ValuePtr PromiseResolverGetPromise(ValuePtr ptr) { Local resolver = value.As(); Local promise = resolver->GetPromise(); m_value* promise_val = new m_value; + promise_val->id = 0; promise_val->iso = iso; promise_val->ctx = ctx; promise_val->ptr = @@ -1475,6 +1510,7 @@ RtnValue PromiseThen(ValuePtr ptr, int callback_ref) { return rtn; } m_value* result_val = new m_value; + result_val->id = 0; result_val->iso = iso; result_val->ctx = ctx; result_val->ptr = @@ -1508,6 +1544,7 @@ RtnValue PromiseThen2(ValuePtr ptr, int on_fulfilled_ref, int on_rejected_ref) { return rtn; } m_value* result_val = new m_value; + result_val->id = 0; result_val->iso = iso; result_val->ctx = ctx; result_val->ptr = @@ -1533,6 +1570,7 @@ RtnValue PromiseCatch(ValuePtr ptr, int callback_ref) { return rtn; } m_value* result_val = new m_value; + result_val->id = 0; result_val->iso = iso; result_val->ctx = ctx; result_val->ptr = @@ -1546,6 +1584,7 @@ ValuePtr PromiseResult(ValuePtr ptr) { Local promise = value.As(); Local result = promise->Result(); m_value* result_val = new m_value; + result_val->id = 0; result_val->iso = iso; result_val->ctx = ctx; result_val->ptr = @@ -1580,6 +1619,7 @@ RtnValue FunctionCall(ValuePtr ptr, ValuePtr recv, int argc, ValuePtr args[]) { return rtn; } m_value* rtnval = new m_value; + rtnval->id = 0; rtnval->iso = iso; rtnval->ctx = ctx; rtnval->ptr = Persistent>(iso, result); @@ -1599,6 +1639,7 @@ RtnValue FunctionNewInstance(ValuePtr ptr, int argc, ValuePtr args[]) { return rtn; } m_value* rtnval = new m_value; + rtnval->id = 0; rtnval->iso = iso; rtnval->ctx = ctx; rtnval->ptr = Persistent>(iso, result); @@ -1611,6 +1652,7 @@ ValuePtr FunctionSourceMapUrl(ValuePtr ptr) { Local fn = Local::Cast(value); Local result = fn->GetScriptOrigin().SourceMapUrl(); m_value* rtnval = new m_value; + rtnval->id = 0; rtnval->iso = iso; rtnval->ctx = ctx; rtnval->ptr = Persistent>(iso, result); diff --git a/v8go.h b/v8go.h index bd492c55..b20daca4 100644 --- a/v8go.h +++ b/v8go.h @@ -166,6 +166,7 @@ extern void CPUProfileDelete(CPUProfile* ptr); extern ContextPtr NewContext(IsolatePtr iso_ptr, TemplatePtr global_template_ptr, int ref); +extern int ContextRetainedValueCount(ContextPtr ctx); extern void ContextFree(ContextPtr ptr); extern RtnValue RunScript(ContextPtr ctx_ptr, const char* source, @@ -207,6 +208,7 @@ extern RtnValue NewValueBigIntFromWords(IsolatePtr iso_ptr, int sign_bit, int word_count, const uint64_t* words); +void ValueRelease(ValuePtr ptr); extern RtnString ValueToString(ValuePtr ptr); const uint32_t* ValueToArrayIndex(ValuePtr ptr); int ValueToBoolean(ValuePtr ptr); diff --git a/value.go b/value.go index c4ba9acf..61d4deec 100644 --- a/value.go +++ b/value.go @@ -531,6 +531,11 @@ func (v *Value) IsProxy() bool { return C.ValueIsProxy(v.ptr) != 0 } +// Release this value. Using the value after calling this function will result in undefined behavior. +func (v *Value) Release() { + C.ValueRelease(v.ptr) +} + // IsWasmModuleObject returns true if this value is a `WasmModuleObject`. func (v *Value) IsWasmModuleObject() bool { // TODO(rogchap): requires test case