[OpenSmalltalk/opensmalltalk-v
Locked
[OpenSmalltalk/opensmalltalk-v
 
|
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
File ChangesPatch Links:
— |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
@krono commented on this pull request. In platforms/iOS/vm/OSX/sqSqueakOSXApplication+events.m: > @@ -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: ⬇️ Suggested change- NSMutableArray *alienEventQueue = [[NSMutableArray alloc] init]; + NSMutableArray *alienEventQueue = AUTORELEASEOBJ([[NSMutableArray alloc] init]); — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
@krono commented on this pull request. In platforms/iOS/vm/OSX/sqSqueakOSXApplication+events.m: > - }
-
- // 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 ⬇️ Suggested change- } + } + RELEASEOBJ(alienEventQueue) — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
I used 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 — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
the — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
I do not fully get the idea of — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
Yea, don't So, when ARC is present (it mostly is nowadays), both macros do exaclty nothing. — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
Hm, where — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
Don't know. That's why I tagged @johnmci — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
@johnmci requested changes on this pull request. Ok, as @krono pointed out 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" — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
> Ok, as @krono pointed out > NSMutableArray *alienEventQueue = AUTORELEASEOBJ([[NSMutableArray alloc] init]); > with the autoreleasepool. Do we need the `RELEASEOBJ` in this case? — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
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? .... ‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, February 28, 2019 9:59 AM, Tobias Pape <[hidden email]> wrote:
|
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
John, thank you for clearing up what's going on BTW: could you open an issue for the autoreleasepool-on-callout? — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
@krono commented on this pull request. In platforms/iOS/vm/OSX/sqSqueakOSXApplication+events.m: > - }
-
- // 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. — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
I will be at Camp Smalltalk at end of March, likely will work on this then. — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
@johnmci can you review this now? — |
Re: [OpenSmalltalk/opensmalltalk-v
|
|
In reply to this post by David T Lewis
Merged #373 into Cog. — |
| Free forum by Nabble | Edit this page |