Wrap Apple disk refreshing in autorelease pool to avoid system caching #1344
Conversation
complexspaces
force-pushed
the
pool-apple-disk-refreshing
branch
from
d83ded6
to
811701b
Compare
|
Just one change but otherwise looks good to me. 👍 |
complexspaces
force-pushed
the
pool-apple-disk-refreshing
branch
from
811701b
to
882330c
Compare
complexspaces
force-pushed
the
pool-apple-disk-refreshing
branch
from
882330c
to
62a02de
Compare
| /// You must not return an Objective-C object that is autoreleased from this function since it | ||
| /// will be freed before usable. | ||
| unsafe fn with_autorelease<T, F: FnOnce() -> T>(call: F) -> T { | ||
| struct DrainPool { |
There was a problem hiding this comment.
What's the point of having this struct? Couldn't both ffi functions be called before and after call()?
There was a problem hiding this comment.
This to to avoid any unnecessary leaking if get_list (or any other function in the future) panics and unwinds the stack. Without it the pop function wouldn't be called in these cases, leading to leaks. With this, the destructor will be called in the process. A lot of programs/libraries wrap calls in std::panic::catch_unwind so a panic here is not guaranteed to terminate the thread (and therefore free its memory) without something like this.
There was a problem hiding this comment.
Hum... I wonder here if it isn't too much considering this is sysinfo code only... Well, I'll let it up to you. If you want to stick with this version, please add a comment on the struct explaining why we do it this way please.
There was a problem hiding this comment.
I added the comment to provide context to future readers of this code.
complexspaces
force-pushed
the
pool-apple-disk-refreshing
branch
from
62a02de
to
5f78295
Compare
|
Thanks a lot! I'll make a release shortly. |
This PR implements a solution for #1282 by adding an Objective-C autorelease pool around the
CoreFoundationFFI callssysinfouses to obtain disk information. This was chosen over a tempoary thread because spawning a thread is a much heavier operation (shared memory, stack allocations, etc). It also seems better then a worker thread because longer-lived apps that don't call this operation much might not want an extra thread living forever. In addition to all these approaches, if people are usingsysinfocode in a traditional cocoa app then this thread would be extra pointless as the built-in runloops would be clearing autorelease'd values out already.In the future this pool could be moved to a more reusable context to support multiple refreshes before draining, but this is a performance issue and not a correctness one. Therefore, it is left as future work.
Closes #1282