Fix incorrect release pattern in macOS/iOS texture loading code #6495
Conversation
`NSData`/`NSMutableData` provide constructors which internally mark them for auto-release pool. New obj-c rule of thumb: If an object is created and auto-released, releasing them manually still keeps them in the pool, and the pool will still try to release the object despite being released already, and end up causing a segmentation fault error. Any object created with a convenient constructor is marked as auto-released (see https://stackoverflow.com/a/24807570), and therefore methods to manually release them should NOT be exposed.
We may want to also consider wrapping the entire SDL window run loop with an `NSAutoreleasePool`. One for a rainy day.
|
|
||
| namespace osu.Framework.Platform.Apple.Native | ||
| { | ||
| internal readonly struct NSAutoreleasePool : IDisposable |
There was a problem hiding this comment.
Was this class auto-generated? I'm just curious as to the structure of the code here (ie. a constructor we never call, private method alloc which is only used in one place, conflicting name with iOS class which behaves with nuanced differences).
There was a problem hiding this comment.
No I've wrote it manually. The constructor is used in alloc(), a separate method is created for alloc() purely as a personal preference. The difference between this and iOS cannot easily be circumvented unless we change everything to act as C# class objects.
I'm still envisioning a future where we remove all of this native interop code and use .NET's SDK implementations, but that future does not look to be near from recent attempts.
This is caused by the fact that the allocated
NSMutableDataobject in the loading process is manually released whilst internally marked as auto-released (it is marked as auto-released because it is created using a convenience constructor, read https://stackoverflow.com/a/24807570 for more info (not owning the object essentially means the object itself is queued in the active auto-release pool).To fix this in an obj-c friendly way, interop for
NSAutoreleasePoolis added and used in places whereNSDatais allocated, and the method to manually releaseNSDatais removed as incorrect practice. I've also updated iOS code accordingly as it is generally better to have a local autorelease pool surrounding a code block which may allocate a large portion of memory.I can confirm the segfault error is fixed by powering up
TestSceneTexturePerformanceand enableunique textureswhile the sprite count is set to 1000. On master, doing that then closing the game causes a segfault, whereas on this PR the game closes gracefully.I have profiled between master and this PR so as to ensure no performance overhead is caused by initialising an auto-release pool. Reported times are almost identical.
I have also stressed out the code by a for-loop in
TestSceneTexturePerformance.load. No sign of memory leak as far as I can tell. All I can see is the game accumulating up to 2 GB of memory usage, and that is due to how ImageSharp allocates space for theImagecreated in each time a texture was loaded.