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.

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

v8.Value becomes manually releaseable #361

Merged
merged 7 commits into from
Jan 19, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
6 changes: 6 additions & 0 deletions context.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 7 additions & 0 deletions function_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
38 changes: 38 additions & 0 deletions function_template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,44 @@ 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())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to keep the test output clean, please consider removing any unneeded print statements here. I know the test is trying to show a print implementation but we should probably check the value here, not print it out. Same thing below.

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() != 5 {
t.Errorf("expected 5 retained values, got: %d", ctx.RetainedValueCount())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is failing for me locally:

go test
# rogchap.com/v8go
v8go.cc: In function ‘void ContextFree(ContextPtr)’:
v8go.cc:620:13: warning: structured bindings only available with ‘-std=c++17’ or ‘-std=gnu++17’
  620 |   for(auto& [key, value] : ctx->vals) {
      |             ^
[foo bar 0 1]
[foo bar 0 1]
--- FAIL: TestFunctionTemplate_generates_values (0.02s)
    function_template_test.go:66: expected 5 retained values, got: 1
FAIL
exit status 1
FAIL	rogchap.com/v8go	7.678s

Can you repro?

}
}

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)", "")
if ctx.RetainedValueCount() != 0 {
t.Errorf("expected 0 retained values, got: %d", ctx.RetainedValueCount())
}
}

func TestFunctionTemplateGetFunction(t *testing.T) {
t.Parallel()
Expand Down
33 changes: 28 additions & 5 deletions v8go.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <sstream>
#include <string>
#include <vector>
#include <unordered_map>

#include "_cgo_export.h"

Expand All @@ -26,12 +27,14 @@ const int ScriptCompilerEagerCompile = ScriptCompiler::kEagerCompile;

struct m_ctx {
Isolate* iso;
std::vector<m_value*> vals;
std::unordered_map<long, m_value*> vals;
std::vector<m_unboundScript*> unboundScripts;
Persistent<Context> ptr;
long nextValId;
};

struct m_value {
long id;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a constructor here to enforce that this member is initialized to zero (eg, m_value() : id(0) { }). Otherwise, we have to depend on the caller to invoke like new m_value() instead of new m_value and everybody here is doing the latter. The reason I think this is that we explicitly check whether id is zero further down.

Isolate* iso;
m_ctx* ctx;
Persistent<Value, CopyablePersistentTraits<Value>> ptr;
Expand Down Expand Up @@ -119,7 +122,10 @@ m_value* tracked_value(m_ctx* ctx, m_value* val) {
// Go <--> C, which would be a significant change, as there are places where

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔕 This to-do can probably be revised.

// 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);
if (val->id == 0) {
val->id = ctx->nextValId++;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? The initial value will be 0, not 1. It felt wrong because the conditional would be true for the initial call and the second call but never any subsequent calls.

I think this should be preincrement. vals is an unordered map now so I don't think we care about filling in from key 0.

ctx->vals[val->id] = val;
}

return val;
}
Expand Down Expand Up @@ -600,16 +606,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& [key, value] : ctx->vals) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting a warning here:

v8go.cc: In function ‘void ContextFree(ContextPtr)’:
v8go.cc:620:13: warning: structured bindings only available with ‘-std=c++17’ or ‘-std=gnu++17’
  620 |   for(auto& [key, value] : ctx->vals) {

I think this is only a c++14 part of the project.

for(auto it = ctx->vals.begin(); it != ctx->vals.end(); ++it) {
  it->second->ptr.Reset();
  delete it->second;
}

value->ptr.Reset();
delete value;
}
ctx->vals.clear();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔕 I don't know about perf but I think it'd be clearer to call ValueRelease() (but be careful about modifying while looping).


for (m_unboundScript* us : ctx->unboundScripts) {
us->ptr.Reset();
Expand Down Expand Up @@ -761,6 +773,17 @@ 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;
Expand Down
2 changes: 2 additions & 0 deletions v8go.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔕 Maybe ctx_ptr and val_ptr to match other APIs.

extern void ContextFree(ContextPtr ptr);
extern RtnValue RunScript(ContextPtr ctx_ptr,
const char* source,
Expand Down Expand Up @@ -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);
Expand Down
5 changes: 5 additions & 0 deletions value.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down