Fixes a memory leak in the macOS event loop, see discussion on mailing list http://lists.squeakfoundation.org/pipermail/vm-dev/2019-February/030595.html You can view, comment on, or merge this pull request online at:
https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373
-- Commit Summary --
* fix event memory leak
-- File Changes --
M platforms/iOS/vm/OSX/sqSqueakOSXApplication+events.m (18)
-- Patch Links --
https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373.patch https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373.diff
krono commented on this pull request.
@@ -73,6 +73,8 @@ @implementation sqSqueakOSXApplication (events)
// If the event does not correspond to this window, we take it from the event queue anyways and re-post it afterwards // This gives other windows the opportunity to consume their events - (void) pumpRunLoopEventSendAndSignal:(BOOL)signal { + + @autoreleasepool { NSEvent *event; NSMutableArray *alienEventQueue = [[NSMutableArray alloc] init];
@johnmci tries to make sure that everything builds on ARC and non-ARC, so maybe do this: ```suggestion NSMutableArray *alienEventQueue = AUTORELEASEOBJ([[NSMutableArray alloc] init]); ```
krono commented on this pull request.
}
- - // Put back in the queue all events that did not belong to this window - // They will be managed by other windowing systems - // (or by a subsequent invocation to this function) - while ((event = [alienEventQueue firstObject])) { - [NSApp postEvent: event atStart: NO]; - [alienEventQueue removeObject: event]; + } + // Put back in the queue all events that did not belong to this window + // They will be managed by other windowing systems + // (or by a subsequent invocation to this function) + while ((event = [alienEventQueue firstObject])) { + [NSApp postEvent: event atStart: NO]; + [alienEventQueue removeObject: event]; + }
…and this ```suggestion } RELEASEOBJ(alienEventQueue) ```
I used `@autoreleasepool` because it is used elsewhere: https://github.com/OpenSmalltalk/opensmalltalk-vm/search?q=autoreleasepool&a...
Having said that, I have virtually no experience with Objective-C and on the coding conventions for the VM, so expert opinions are highly appreciated. Also, we need to make sure that the solution does not have unwanted side-effects. If I am understanding the code correctly, we only have to make sure that `alienEventQueue` is eventually released.
the `@autoreleasepool` is ok, leave it there. Just maybe add the other things too :)
I do not fully get the idea of `AUTORELEASEOBJ` and friends, this needs to be tested again. Guille mentioned that `[alienEventQueue release]` lead to segfaults: http://lists.squeakfoundation.org/pipermail/vm-dev/2019-February/030607.html. Not sure if this might cause issues.
Yea, don't `release` manually. The macros get populated depending on availability of ARC: https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/Cog/platforms/iOS/vm/...
So, when ARC is present (it mostly is nowadays), both macros do exaclty nothing.
Hm, where `@autoreleasepool` is used, `RELEASEOBJ` is mostly not used. Isn't `@autoreleasepool` enough, even for non-ARC?
Don't know. That's why I tagged @johnmci
johnmci requested changes on this pull request.
Ok, as @krono pointed out NSMutableArray *alienEventQueue = AUTORELEASEOBJ([[NSMutableArray alloc] init]); with the autoreleasepool.
I'm not sure anything is build without ARC anymore, but it should be there to be correct for people who need to build on older platforms.
AUTORELEASEOBJ with ARC does nothing, and the autoreleasepool wrapper then is freeing autorelease objects in the scope of this routine. Since it is pumped from the VM processing requests there isn't a wrapping autorelease pool higher up in the stack. Easy test is to do invoke the "About Menu"
Ok, as @krono pointed out NSMutableArray *alienEventQueue = AUTORELEASEOBJ([[NSMutableArray alloc] init]); with the autoreleasepool.
Do we need the `RELEASEOBJ` in this case?
No you do not need the RELEASEOBJ
Before ARC there was a lot of work involved in managing memory. For situation where you allocated or made a copy of an object (various rules).
X = [NSMutableArray alloc] init]);
Then you had to ensure you released the object when you were done via release, or tag it via autorelease
For the case where an object was created but autoreleased you then had to ensure you retain it either via an explicit retain or use of a retain property,
So saying self.retainedObject = [NSArray array]; //returns autorelease object versus retainedObject = [[NSArray alloc] init];
had very different meaning as the self. would do the assignment and retain, but use without self. would not do the retain, which was not needed as the alloc/init gives a retain count of 1.
Post ARC the compiler is aware of the scope of the life of the object and inserts retain/release as needbe.
That all said what is going on is very subtle. The actual problem is that the X = [NSMutableArray alloc] init]); is handled OK by ARC because the object is autoreleased when the method ends.
However the question is well what happens to the auto release pool? When does that get cleaned up?
That is the actual problem, and adding the auto-release pool cleans up the NSEvents that are creating the majority of the garbage, since there is no autorelease wrapper for the callout from the interp.c code.
There likely are others, mmm ioSetCursor Maybe there needs to be a general rule that any call outs to obj logic from interp.c have to have an autorelease pool?
.... John M. McIntosh. Corporate Smalltalk Consulting Ltd https://www.linkedin.com/in/smalltalk
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, February 28, 2019 9:59 AM, Tobias Pape notifications@github.com wrote:
Ok, as @krono pointed out NSMutableArray *alienEventQueue = AUTORELEASEOBJ([[NSMutableArray alloc] init]); with the autoreleasepool.
Do we need the `RELEASEOBJ` in this case?
— You are receiving this because you are subscribed to this thread. Reply to this email directly, [view it on GitHub](https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-4683...), or [mute the thread](https://github.com/notifications/unsubscribe-auth/AhLyW0_DSJBNfTEFzQknE8qU_r...).
John, thank you for clearing up what's going on
BTW: could you open an issue for the autoreleasepool-on-callout?
krono commented on this pull request.
}
- - // Put back in the queue all events that did not belong to this window - // They will be managed by other windowing systems - // (or by a subsequent invocation to this function) - while ((event = [alienEventQueue firstObject])) { - [NSApp postEvent: event atStart: NO]; - [alienEventQueue removeObject: event]; + } + // Put back in the queue all events that did not belong to this window + // They will be managed by other windowing systems + // (or by a subsequent invocation to this function) + while ((event = [alienEventQueue firstObject])) { + [NSApp postEvent: event atStart: NO]; + [alienEventQueue removeObject: event]; + }
Ok, this no.
I will be at Camp Smalltalk at end of March, likely will work on this then.
@johnmci can you review this now?
Merged #373 into Cog.
vm-dev@lists.squeakfoundation.org
-
Eliot Miranda -
John M McIntosh -
johnmci
-
maenu
-
Tobias Pape