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

integrate objc release with Go garbage collector #49

Closed
progrium opened this issue May 12, 2021 · 8 comments
Closed

integrate objc release with Go garbage collector #49

progrium opened this issue May 12, 2021 · 8 comments
Labels
help wanted Extra attention is needed

Comments

@progrium
Copy link
Owner

as mentioned in #12, not using a pointer for type object means its passed as a value. i believe the suggestion was to use it as a pointer type so we could use SetFinalizer effectively. however, this brings up some questions.

since sendMsg creates (maybe all) object values, would we want to track those pointer values so there is only one object in go holding reference to an objc pointer?

would this negate the need for using autoreleasepool at all?

@progrium progrium added the help wanted Extra attention is needed label May 12, 2021
@mgood
Copy link
Sponsor Collaborator

mgood commented May 13, 2021

IIUC I do think we'll want to structure it so that there's only one Go value holding each objc reference. Though if multiple objc calls return references to the same object, each reference would be unique. So as each Go reference is GC-ed, it will release the objc, decrementing its reference count, and eventually freeing the object.

However, it doesn't sound like this removes the need for an autoreleasepool. Releasing the Go references would handle objects that are passed back to Go as a return value. However, an objc call may allocate additional objects that are added to the pool, but not directly referenced by the return value. So, calling release on the return value would not also release those values.

I'll follow up on #12 with some additional thoughts.

@mgood mgood mentioned this issue May 20, 2021
@mgood
Copy link
Sponsor Collaborator

mgood commented May 25, 2021

Hmm, so I think there's an issue here with how the package currently handles some primitive values. E.g. calls to Send() that return raw numeric values still wrap it in an Object, where the caller is expected to use .Int() to convert it. But for these, the wrapping Object instance doesn't know that the ptr value is really just a number and not a pointer. So calling Release() in the finalizer would crash.

So, it seems like this is first going to require some changes to how it wraps return values to distinguish which are wrapping pointers vs raw values.

@progrium
Copy link
Owner Author

Yea, there is some simplicity to just assuming objects, esp since most of the APIs are just returning objects. In a way, letting the caller decide to Release gets around this as well. I'm ok with the idea of saying all return values, whether values or objects, will be objects, but once it starts impacting any magic we do with releasing it makes things more complicated.

Also keep in mind we will have access to schema information for APIs, so when we create wrappers in Go we can decide based on the return value type to do one thing or another.

@progrium
Copy link
Owner Author

We could say that Go GC integration via finalizer is only done with wrapped/generated code and do it there and not at the objc package level.

@mgood
Copy link
Sponsor Collaborator

mgood commented May 26, 2021

We could say that Go GC integration via finalizer is only done with wrapped/generated code and do it there and not at the objc package level.

Yeah, that could be interesting, and also led me to thinking about a simpler way to just wrap the Object interface to add the GC functionality in #51.
We can leave the objc package a little lower-level and move the automated memory management into the wrapper layer with some of the helpers.

@mgood mgood changed the title objc: use pointer value for object? integrate objc release with Go garbage collector Oct 21, 2021
@mgood
Copy link
Sponsor Collaborator

mgood commented Oct 21, 2021

I've retitled this to better reflect the goal. A pointer is necessary for the GC so that we pass around a single instance in memory, though with an approach like #51 it's possible to add that GC support to a wrapper.

However, looking at this again, I do think we'd need to change how we handle Objective-C pointers to ensure that re-wrapping the object in a different type doesn't trigger a GC of the original.

For example you may need code like this to wrap some reference as an NSString:

b := NSString_fromRef(a)
// equivalent to:
// b := NSString_fromPointer(unsafe.Pointer(a.Pointer()))

If there are no further references to a, it could be garbage collected, triggering the finalizer even though b is expected to be an equivalent reference.

So, we'd want a different way of re-wrapping a "handle" that refers to an object without losing the original Go pointer that holds it.

@mgood
Copy link
Sponsor Collaborator

mgood commented Oct 22, 2021

@ lunixbochs I'm going to continue the discussion from this comment here. #50 (comment)

Note that a big reason to not use implicit autoreleasepool, is that it depends on LockOSThread() to work correctly, and a user can easily starve the go thread pool if they don't know that. If you warn them to use the autoreleasepool API prominently, you also have a chance to tell them about the caveats and manage expectations.

Yeah, my ideal about using autoreleasepools implicitly may be too idealistic, but I was kind of wondering if by combining that with the GC for releasing objects referenced on the Go side we might be able to handle most memory management needs automatically.

The thread pool is an interesting point, do you have some more info on when that might happen? I haven't tested this extensively, but I was re-reading the docs on SetMaxThreads to try to better understand the limitations:

A Go program creates a new thread only when a goroutine is ready to run but all the existing threads are blocked in system calls, cgo calls, or are locked to other goroutines due to use of runtime.LockOSThread.
https://pkg.go.dev/runtime/debug@go1.17.2#SetMaxThreads

Based on that it sounds like cgo is also locked to a single thread. Would calling LockOSThread() around a single cgo call (as would happen with the implicit autoreleasepool idea) have much additional impact if the cgo call would also be locked to the thread?

Implicitly creating autoreleasepools for every method call still have other issues, but do you have more info on how LockOSThread interacts here and if it presents other problems I'm missing?

@progrium
Copy link
Owner Author

Going to close this for now since it should all be covered now:
https://github.com/progrium/macdriver/blob/main/docs/memorymanagement.md

But we can open a new issue for anything else from here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants