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

fix event memory leak #373

Merged
merged 1 commit into from Jun 28, 2019

Conversation

@maenu
Copy link
Contributor

commented Feb 28, 2019

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

@@ -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];

This comment has been minimized.

Copy link
@krono

krono Feb 28, 2019

Member

@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]);
while ((event = [alienEventQueue firstObject])) {
[NSApp postEvent: event atStart: NO];
[alienEventQueue removeObject: event];
}

This comment has been minimized.

Copy link
@krono

krono Feb 28, 2019

Member

…and this

Suggested change
}
}
RELEASEOBJ(alienEventQueue);

This comment has been minimized.

Copy link
@krono

krono Feb 28, 2019

Member

Ok, this no.

@maenu

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

I used @autoreleasepool because it is used elsewhere: https://github.com/OpenSmalltalk/opensmalltalk-vm/search?q=autoreleasepool&unscoped_q=autoreleasepool

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.

@krono

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

the @autoreleasepool is ok, leave it there. Just maybe add the other things too :)

@maenu

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

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.

@krono

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

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/SqueakPureObjc_Prefix.pch

So, when ARC is present (it mostly is nowadays), both macros do exaclty nothing.

@maenu

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2019

Hm, where @autoreleasepool is used, RELEASEOBJ is mostly not used. Isn't @autoreleasepool enough, even for non-ARC?

@krono

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Don't know. That's why I tagged @johnmci

Copy link
Contributor

left a comment

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"

@krono

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

@OpenSmalltalk-Bot

This comment has been minimized.

Copy link

commented Feb 28, 2019

@krono

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

John, thank you for clearing up what's going on

BTW: could you open an issue for the autoreleasepool-on-callout?

@johnmci

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2019

I will be at Camp Smalltalk at end of March, likely will work on this then.

@maenu maenu referenced this pull request Apr 9, 2019
@eliotmiranda

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

@johnmci can you review this now?

@johnmci johnmci merged commit 3f544c0 into OpenSmalltalk:Cog Jun 28, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.