Bug 5036 - NSActionDispatcher could be disposed instead of left to the GC.
Summary: NSActionDispatcher could be disposed instead of left to the GC.
Status: RESOLVED FIXED
Alias: None
Product: iOS
Classification: Xamarin
Component: Xamarin.iOS.dll ()
Version: 5.2
Hardware: PC Mac OS
: --- normal
Target Milestone: Untriaged
Assignee: Bugzilla
URL:
Depends on:
Blocks:
 
Reported: 2012-05-11 16:48 UTC by Chris Toshok
Modified: 2013-09-27 12:14 UTC (History)
2 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Chris Toshok 2012-05-11 16:48:25 UTC
in BeginInvokeOnMainThread, monotouch currently does:


1.  NSActionDispatcher nSActionDispatcher = new NSActionDispatcher (action);
2. Messaging.void_objc_msgSend_intptr_intptr_bool (nSActionDispatcher.Handle, Selector.PerformSelectorOnMainThreadWithObjectWaitUntilDone, NSActionDispatcher.Selector.Handle, nSActionDispatcher.Handle, false);
3. GC.KeepAlive (nSActionDispatcher);

1) that keepalive isn't necessary at all, a PerformSelectorOnMainThreadWithObjectWaitUntilDone will retain the object and flip the toggleref to strong.
2) you could replace the keepalive with nsSActionDispatcher.Dispose() for the same reason, and then the object would go away when the mainloop is done with it, instead of waiting until the next gc cycle.
Comment 1 Rolf Bjarne Kvinge [MSFT] 2012-05-11 19:33:42 UTC
2) It is not possible to call Dispose for BeginInvokeOnMainThread, once Dispose is called the managed object may be freed at any time (it is possible to add a Dispose call for InvokeOnMainThread though). We can however call Dispose once the callback has been called for the async case.
Comment 2 Rolf Bjarne Kvinge [MSFT] 2012-05-11 20:07:33 UTC
Fixed in monotouch master (e91fa1a and 70aee7a) and maccore master (d92fc6a8).

InvokeOnMainThread calls Dispose on the NSActionDispatcher after calling the selector.
BeginInvokeOnMainThread calls Dispose after the action has been dispatched.
Comment 3 Chris Toshok 2012-08-07 19:54:24 UTC
now that the dispose has been reverted, I suppose a little context is in order.

this is from my may-11-2012 #monotouch irc logs:

[12:49:39] toshok: so this is why sgen sucks:  http://www.hungry.com/~toshok/this-sucks.png
[12:49:58] toshok: expected: "yay, that graph means we're freeing memory!"
[12:50:15] toshok: perhaps unexpected: "hey, we had all that garbage laying around for a long while, and we decided to collect it all while the user was scrolling"
[12:50:44] toshok: that's a drop of about 2 megs, btw

the root of that problem was NSActionDispatcher because sgen takes too long to collect NSObject subclasses and the tree of reachable objects from the delegates held in NSActionDispatcher was *massive*.

ios is a hard target for gc at all, particularly one where heap sizes are allowed to grow large.  you have to keep the heap relatively clear so that you don't get hit with a major collection at the wrong time (while scrolling, e.g. which leads to really terrible experiences), and at the same time you have to keep pause times down when you do collect.  You also have to keep the heap small so that you aren't killed while swapped out, since you can't gc then at all.

If there are objects not even exposed to users with known lifetimes that are known contributors to larger heaps, it seems a no-brainer to help the GC out.  Less garbage = smaller heaps and fewer/shorter pauses.

This .Dispose helped a *ton*
Comment 4 Chris Toshok 2012-08-07 20:22:10 UTC
i suppose nulling the handle takes care of breaking the chain.  but still, if using the GC pipeline is a concern, why leave the null in? :)

I assert that if that .Dispose is causing issues with your runtime instrumenting, something is wrong with the runtime instrumenting, not the .Dispose..  it worked before, after all?
Comment 5 Rolf Bjarne Kvinge [MSFT] 2013-09-27 12:14:10 UTC
The Dispose calls were re-added some time ago.

https://github.com/mono/maccore/blob/master/src/Foundation/NSAction.cs#L102