Macos multi windows leak #481

bogdad · 2018-04-22T20:44:45Z

made multi window leak less memory

created a simple test which spawns windows and recreates them on "return" key press, the title of the window is the number of windows created.

before the change

after the change:

and more significant:

total persisted before:

and total persisted after

The gist of the change is the adding NSAutorelease pools, as in most places where we are calling objc it uses NSObject:autorelease which puts the object in the top most autorelease pool on the stack, or it is never released if no such autorelease pool exist,
or if the topmost autorelease pool is outside of the event loop, i.e. gets freed after the thread terminates.

This is continuation of #303 which was an incorrect attempt to do the same thing.
This probably fixes #158

I know its hard to check that without instruments and looking at the lists of allocations,
not sure how to best approach approving this pr

edwin0cheng · 2018-04-23T16:27:25Z

Just a little reminder, same as #428:

Due to the recent changes in the Rust compiler, unconstrained type
variables are now deduced to ! instead of (). There are some
occurrences where msg_send! is used without constraining its return
type (relying on the assumption that they would be deduced to be ()).
As a result, the macOS port of winit stopped working in nightly.

So all msg_send! call should be set a return value expliclty, just like this:

let _ : () = msg_send![...];

bogdad · 2018-04-24T09:01:10Z

@edwin0cheng hi! thank you, fixed!

francesca64 · 2018-04-28T04:02:10Z

Alright, I'm back on macOS, so I can review this now. First of all, those statistics look great!

I can still reproduce #158 on this branch, but after rebasing it seems completely fixed. Be sure to double-check that.

Before this can merge:

examples/recreate_window_leak.rs needs to be removed, since it's not useful for users. You can embed it in your PR description or as a gist if you still want it to be accessible.
This should have a CHANGELOG entry. I know it's not straightforward to write one for something abstract like this, but a good overview of what this improves will both A) make the next winit release seem cooler, and B) give people an idea of what things to check if they experience regressions.

Note that at the time of writing, I haven't actually finished reviewing this yet, so there could be other things. I just wanted to let you know this was being worked on.

bogdad · 2018-04-28T04:29:36Z

Hi! Thank you for working on it, everything seems reasonable, will do. I missed checking 158, nice it is fixed! Please give me few days to update.

…

On Sat, 28 Apr 2018 at 06:02, Francesca Frangipane ***@***.***> wrote: Alright, I'm back on macOS, so I can review this now. First of all, those statistics look great! I can still reproduce #158 <#158> on this branch, *but* after rebasing it seems completely fixed. Be sure to double-check that. Before this can merge: - examples/recreate_window_leak.rs needs to be removed, since it's not useful for users. You can embed it in your PR description or as a gist if you still want it to be accessible. - This should have a CHANGELOG entry. I know it's not straightforward to write one for something abstract like this, but a good overview of what this improves will both A) make the next winit release seem cooler, and B) give people an idea of what things to check if they experience regressions. Note that at the time of writing, I haven't actually finished reviewing this yet, so there could be other things. I just wanted to let you know this was being worked on. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#481 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEBGkIyTgmlxZRwgs6FiCpUUXe4Ro81ks5ts-nDgaJpZM4TfBSf> .

as setDelegate:nil autoreleases WindowDelegate during work. Added NSAutoReleasepool for Window2::Create, as it uses autorelease on objects while doing work. Added NSAutoreleasepool for Window2::Drop as nswindow::close uses autorelease on objects. Added NSAutoreleasepool for IdRef. Moved Window2 WinitWindow objc class to a static var, as we are creating multiple windows.

bogdad · 2018-04-28T12:22:53Z

Rebased, added change log entry (not really sure it is a good one, please advise), checked the multiwindow example,

this shows amount of alive window objects

seems they are released as I close them

this is how it looks like on a master:

tried to repro 158 with multi desktops but could not.

There is still some bytes leaked, namely some NSStrings and titles, but they are neglible in total memory usage, lets deal with them in the future.

Can also confirm that this branch (without rebasing) alone did not fix 158 issue, which is interesting,
trying to figure out why meanwhile. (this is the branch with the previous state on which multiwindow example does not release windows in time https://github.com/bogdad/winit/tree/wo_rebasing)

bogdad · 2018-04-28T12:25:39Z

src/platform/macos/events_loop.rs

@@ -94,7 +94,7 @@ impl Shared {
        if let Ok(mut windows) = self.windows.lock() {
            windows.retain(|w| match w.upgrade() {
                Some(w) => w.id() != id,
-                None => true,
+                None => false,


dont need to keep dead week ptrs here

francesca64 · 2018-04-28T14:36:07Z

src/platform/macos/window.rs

@@ -452,9 +452,15 @@ impl WindowDelegate {
        unsafe {
            let delegate = IdRef::new(msg_send![WindowDelegate::class(), new]);

+            // setDelegate uses autorelease on objects,
+            // so need autorelease
+            let autoreleasepool = NSAutoreleasePool::new(cocoa::base::nil);


Is there any reason you went with cocoa::base::nil instead of using the imported nil? It's a minor gripe, but having that consistent makes it easier for people to tell the same type is being used throughout.

francesca64 · 2018-04-28T14:38:21Z

Rebased, added change log entry (not really sure it is a good one, please advise)

Thanks. The CHANGELOG entry looks good.

checked the multiwindow example,

Thanks for doing that. It gives me good peace of mind.

Can also confirm that this branch (without rebasing) alone did not fix 158 issue, which is interesting, trying to figure out why meanwhile.

Prior to #476, windows in the multiwindow example weren't dropped until the program exited. Prior to #481, the drop implementation wasn't releasing windows correctly. With both, windows drop at the correct time and are dropped the right way.

francesca64 · 2018-04-28T14:39:12Z

src/platform/macos/window.rs

@@ -638,11 +679,30 @@ impl Window2 {
        }
    }

+    fn class() -> *const Class {
+


Extra newline

francesca64 · 2018-04-28T14:40:39Z

src/platform/macos/window.rs

+            shared.find_and_remove_window(id);
+        }
+
+


Extra newline

francesca64 · 2018-04-28T14:44:28Z

src/platform/macos/window.rs

-            None      => { return Err(OsError(format!("Couldn't create NSApplication"))); },
+            None      => {
+                let _: () = unsafe { msg_send![autoreleasepool, drain] };
+                return Err(OsError(format!("Couldn't create NSApplication"))); },


The }, should be on a newline. (This applies to the 2 matches below as well)

francesca64 · 2018-04-28T14:46:06Z

In terms of functionality, everything looks good. I pretty much just gave you some formatting nitpicks (sorry about those, but I'm hoping to get winit to look more cohesive internally).

francesca64 · 2018-04-28T14:52:48Z

src/platform/macos/window.rs

+        // Remove this window from the `EventLoop`s list of windows.
+        // The destructor order is:
+        // Window ->
+        // Rc<Window2> (makes Weak<..> in shared.windows none) ->


It would also be nice to have none be None, for clarity. We should be good to merge after this.

bogdad · 2018-04-28T15:54:47Z

Fixed all code formatting errors, sorry about that, and thank you for all the work and explanation!

francesca64 · 2018-04-28T16:09:59Z

Thanks!

bogdad mentioned this pull request Apr 22, 2018

winit multiple windows leak macos x #303

Closed

francesca64 mentioned this pull request Apr 28, 2018

[macOS] NSWindows are not released when closed, causing odd zombie window behaviour in some cases. #158

Closed

bogdad added 6 commits April 28, 2018 13:11

adding a multiwindow example

6cfb845

specifying return type for msg_send!

ab4e7a8

removing example/recreate_window_leak.rs

6ea3736

EventLoop, Shared, no need to retain dead weak ptr

dc02950

Change log entry added

69096b6

bogdad force-pushed the macos_window_leak branch from c657566 to 69096b6 Compare April 28, 2018 12:06

bogdad commented Apr 28, 2018

View reviewed changes

added comment about Shared.find_and_remove_window

ebe1312

francesca64 reviewed Apr 28, 2018

View reviewed changes

src/platform/macos/window.rs Outdated

@@ -638,11 +679,30 @@ impl Window2 {

}

}

fn class() -> *const Class {

Copy link

Member

francesca64 Apr 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra newline

francesca64 reviewed Apr 28, 2018

View reviewed changes

src/platform/macos/window.rs Outdated

shared.find_and_remove_window(id);

}

Copy link

Member

francesca64 Apr 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra newline

francesca64 reviewed Apr 28, 2018

View reviewed changes

fixed code style errors

b11259f

francesca64 merged commit 3407a8d into rust-windowing:master Apr 28, 2018

bogdad deleted the macos_window_leak branch June 2, 2018 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Macos multi windows leak #481

Macos multi windows leak #481

bogdad commented Apr 22, 2018 •

edited

Loading

edwin0cheng commented Apr 23, 2018

bogdad commented Apr 24, 2018

francesca64 commented Apr 28, 2018

bogdad commented Apr 28, 2018 via email

bogdad commented Apr 28, 2018 •

edited

Loading

bogdad Apr 28, 2018 •

edited

Loading

francesca64 Apr 28, 2018

francesca64 commented Apr 28, 2018

francesca64 Apr 28, 2018

francesca64 Apr 28, 2018

francesca64 Apr 28, 2018 •

edited

Loading

francesca64 commented Apr 28, 2018

francesca64 Apr 28, 2018

bogdad commented Apr 28, 2018

francesca64 commented Apr 28, 2018

Macos multi windows leak #481

Macos multi windows leak #481

Conversation

bogdad commented Apr 22, 2018 • edited Loading

edwin0cheng commented Apr 23, 2018

bogdad commented Apr 24, 2018

francesca64 commented Apr 28, 2018

bogdad commented Apr 28, 2018 via email

bogdad commented Apr 28, 2018 • edited Loading

bogdad Apr 28, 2018 • edited Loading

Choose a reason for hiding this comment

francesca64 Apr 28, 2018

Choose a reason for hiding this comment

francesca64 commented Apr 28, 2018

francesca64 Apr 28, 2018

Choose a reason for hiding this comment

francesca64 Apr 28, 2018

Choose a reason for hiding this comment

francesca64 Apr 28, 2018 • edited Loading

Choose a reason for hiding this comment

francesca64 commented Apr 28, 2018

francesca64 Apr 28, 2018

Choose a reason for hiding this comment

bogdad commented Apr 28, 2018

francesca64 commented Apr 28, 2018

bogdad commented Apr 22, 2018 •

edited

Loading

bogdad commented Apr 28, 2018 •

edited

Loading

bogdad Apr 28, 2018 •

edited

Loading

francesca64 Apr 28, 2018 •

edited

Loading