[OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

[OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
 

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

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"fix event memory leak (#373)"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
 

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


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@krono commented on #373"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#pullrequestreview-209017358"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#pullrequestreview-209017358", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#pullrequestreview-209017358", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

@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)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@krono commented on #373"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#pullrequestreview-209018067"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#pullrequestreview-209018067", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#pullrequestreview-209018067", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

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.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@maenu in #373: I used `@autoreleasepool` because it is used elsewhere: https://github.com/OpenSmalltalk/opensmalltalk-vm/search?q=autoreleasepool\u0026unscoped_q=autoreleasepool\r\n\r\nHaving 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."}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468249930"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468249930", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468249930", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

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


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@krono in #373: the `@autoreleasepool` is ok, leave it there. Just maybe add the other things too :)"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468250520"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468250520", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468250520", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

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.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@maenu in #373: 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."}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468252869"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468252869", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468252869", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

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.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@krono in #373: Yea, don't `release` manually.\r\nThe macros get populated depending on availability of ARC: https://github.com/OpenSmalltalk/opensmalltalk-vm/blob/Cog/platforms/iOS/vm/SqueakPureObjc_Prefix.pch\r\n\r\nSo, when ARC is present (it mostly is nowadays), both macros do exaclty nothing.\r\n"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468253626"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468253626", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468253626", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

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


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@maenu in #373: Hm, where `@autoreleasepool` is used, `RELEASEOBJ` is mostly not used. Isn't `@autoreleasepool` enough, even for non-ARC?"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468260164"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468260164", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468260164", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

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


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@krono in #373: Don't know. That's why I tagged @johnmci "}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468260399"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468260399", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468260399", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

@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"


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@johnmci requested changes on #373"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#pullrequestreview-209221414"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#pullrequestreview-209221414", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#pullrequestreview-209221414", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

> 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, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@krono in #373: \n\u003e Ok, as @krono pointed out\n\u003e NSMutableArray *alienEventQueue = AUTORELEASEOBJ([[NSMutableArray alloc] init]);\n\u003e with the autoreleasepool.\n\nDo we need the `RELEASEOBJ` in this case?\n\n"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468373330"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468373330", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468373330", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

johnmci
 
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 <[hidden email]> 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, or mute the thread.


Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

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

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


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@krono in #373: John, thank you for clearing up what's going on\r\n\r\nBTW: could you open an issue for the autoreleasepool-on-callout?"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468425292"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468425292", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-468425292", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

@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.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@krono commented on #373"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#discussion_r261369700"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#discussion_r261369700", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#discussion_r261369700", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

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


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSmalltalk/opensmalltalk-vm","title":"OpenSmalltalk/opensmalltalk-vm","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm"}},"updates":{"snippets":[{"icon":"PERSON","message":"@johnmci in #373: I will be at Camp Smalltalk at end of March, likely will work on this then. "}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-471768037"}}}</script> <script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-471768037", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373#issuecomment-471768037", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

@johnmci can you review this now?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373?email_source=notifications\u0026email_token=AIJPEW72L6UADOXYYV73SULP4O2C5A5CNFSM4G2ZWAXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYUOVGI#issuecomment-505997977", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373?email_source=notifications\u0026email_token=AIJPEW72L6UADOXYYV73SULP4O2C5A5CNFSM4G2ZWAXKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYUOVGI#issuecomment-505997977", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>
Reply | Threaded
Open this post in threaded view
|

Re: [OpenSmalltalk/opensmalltalk-vm] fix event memory leak (#373)

Eliot Miranda-4
In reply to this post by Eliot Miranda-4
 

Merged #373 into Cog.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/ld+json">[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373?email_source=notifications\u0026email_token=AIJPEW4BYZKBOHYI5LFDOITP4VIZZA5CNFSM4G2ZWAXKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOSHEVHFI#event-2445890453", "url": "https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/373?email_source=notifications\u0026email_token=AIJPEW4BYZKBOHYI5LFDOITP4VIZZA5CNFSM4G2ZWAXKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOSHEVHFI#event-2445890453", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]</script>