v8.Value becomes manually releaseable

[fizx]

This allows long-lived contexts that call go functions without leaking memory. Typically, you'd want to use the following pattern for go functions that don't allow values to escape.

printfn := v8.NewFunctionTemplate(iso, func(info *v8.FunctionCallbackInfo) *v8.Value {
	defer info.Release()
	fmt.Printf("%+v\n", info.Args())
	return nil
})

[RGood]

This is great if it can be used to mitigate the memleak issues associated with #105

The code looks good, but it would be nice to get a memory profile demonstrating it

[niedzielski]

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.

[niedzielski]

🔕 This to-do can probably be revised.

[niedzielski]

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;
}

[niedzielski]

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?

[niedzielski]

🔕 Maybe ctx_ptr and val_ptr to match other APIs.

[niedzielski]

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.

[niedzielski]

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

[codecov]

Codecov Report

Base: 95.23% // Head: 95.28% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (fbe0f6e) compared to base (1f00b50).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #361      +/-   ##
==========================================
+ Coverage   95.23%   95.28%   +0.04%     
==========================================
  Files          17       17              
  Lines         588      594       +6     
==========================================
+ Hits          560      566       +6     
  Misses         19       19              
  Partials        9        9              

Impacted Files	Coverage Δ
context.go	100.00% <100.00%> (ø)
function_template.go	94.11% <100.00%> (+0.36%)	⬆️
value.go	94.85% <100.00%> (+0.02%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[niedzielski]

🥇

[niedzielski]

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.