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

[bindings/go] Remove finalizers in favor of copying and destroying the native resources immediately #1910

Merged
merged 6 commits into from Apr 16, 2020

Conversation

ryanworl
Copy link
Collaborator

@ryanworl ryanworl commented Jul 29, 2019

Fixes #1340

Except for futureKeyValueArray, all futures which allocate native heap resources (other than the futures themselves) now allocate Go resources and copy from native to Go, then destroy the native heap resources.

This adds an explicit Close() function to RangeIterator which users should call.

I also added an optimization for futureKeyValueArray which allocates one large []byte for the result of each batch and subdivides within that instead of allocating individual []byte for each key and value.

I haven't evaluated this much yet, so I'd like to do that before merging. I'll comment once I have.

Not ready yet.

…rces immediately.

Except for futureKeyValueArray, all futures which allocate native heap resources (other than the futures themselves) will allocate Go resources and copy from native to Go, then destroy the native heap resources.
@alexmiller-apple
Copy link
Contributor

@fdb-build, ok to test

@hgray1 hgray1 added this to the 7.0 milestone Jul 29, 2019
@ryanworl
Copy link
Collaborator Author

@alexmiller-apple is there a reason I can’t view the CI logs?

@alexmiller-apple
Copy link
Contributor

@ryanworl, according to @AlvinMooreSr, we started getting throttled by bintray on uploading them, and the jury is out on when they'll unthrottle us. So it's a known our-side problem for now.

Documentation:

13:13:43 Warning, treated as error:
13:13:43 /foundationdb/documentation/sphinx/source/release-notes.rst:49: ERROR: Broken role invoked

CMake MacOS:

13:14:16 [ 64%] Built target fdb_flow_tester
13:14:16 # github.com/apple/foundationdb/bindings/go/src/fdb
13:14:16 src/github.com/apple/foundationdb/bindings/go/src/fdb/futures.go:370:13: could not determine kind of name for C.fdb_future_get_version
13:14:16 make[2]: *** [bindings/go/pkg/darwin_amd64/github.com/apple/foundationdb/bindings/go/src/fdb.a] Error 2
13:14:16 make[1]: *** [bindings/go/CMakeFiles/fdb_go.dir/all] Error 2

and MacOS is a known issue on all PRs right now.

@alexmiller-apple
Copy link
Contributor

@fdb-build, test macos please

@ryanworl
Copy link
Collaborator Author

That one is a real error from my bringing my changes from 610 to 620.

@@ -366,27 +407,40 @@ type FutureStringSlice interface {

type futureStringSlice struct {
*future
o sync.Once
e error
v []string
Copy link
Contributor

@vishesh vishesh Aug 8, 2019

Choose a reason for hiding this comment

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

I would suggest using once instead of o (once.Do reads much better) and err instead of e in field names (I see err used at other parts of this file). I dunno what v is, which probably is good indicator that it should be renamed too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In that case, it is the value the future will resolve with.

I agree with your assessment about the names, but I don’t know if this PR is the place to resolve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok. I missed that it was that way already in other part of codebase.

@alexmiller-apple
Copy link
Contributor

@ryanworl, we're going to be cutting the 7.0 release sometime over the next few weeks. If you're still interested in working on this, and would like it to get into the 7.0 release, then it'd be good to work towards getting it merged within a couple weeks.

@vishesh
Copy link
Contributor

vishesh commented Apr 16, 2020

@fdb-build, test cmake please

@vishesh
Copy link
Contributor

vishesh commented Apr 16, 2020

@fdb-build test ctest please

@vishesh
Copy link
Contributor

vishesh commented Apr 16, 2020

@fdb-build, test cmake macos please

@vishesh
Copy link
Contributor

vishesh commented Apr 16, 2020

@fdb-build, test ctest please

@vishesh vishesh merged commit cef556b into apple:master Apr 16, 2020
vishesh added a commit to vishesh/foundationdb that referenced this pull request Apr 29, 2020
…inalizers"

This reverts commit cef556b, reversing
changes made to a6fe8c1.
vishesh added a commit to vishesh/foundationdb that referenced this pull request Apr 29, 2020
…inalizers"

This reverts commit cef556b, reversing
changes made to a6fe8c1.
ajbeamon added a commit that referenced this pull request Apr 29, 2020
Revert "Merge pull request #1910 from ryanworl/ryanworl/remove-finali…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Golang: provide explicit methods to "close" objects with native resources
4 participants