Expose [NS|UI]View.Subviews with NSArray variant

[Therzok]

Steps to Reproduce

1. Query NSView.Subviews
2. Observe that every time we call it, we marshal an array

Expected Behavior

We should have an API endpoint that exposes an NSArray.

Actual Behavior

There is none.

Notes

Scenario:

NSView view = ...;
for (int i = 0; i < view.Subviews.Length; ++i) {
    var subView = view.Subviews [i];
    ...
}

The above code would marshal the array twice, every time we iterate. Also, array length queries would have to marshal the array to managed, so simply doing:

bool hasChildren = view.Subviews.Count > 0;

Having an NSArray based overload would avoid that and help for performance with multiple subviews in the NSView.

[chamons]

Monkey Patch: https://gist.github.com/chamons/b76888c90c8154bb87720dda56f3264f

[spouliot]

It's both a general [1] problem (calling multiple time the same property without realizing its cost [2]) and a matter of discoverability of the alternative/better API, i.e. if you know there's a problem it's easy to work around. E.g.

• You can assign Subview to a local variable to avoid the penalty (single call)

var local = v.Subviews;
for (int i = 0; i < local.Length; ++i) {
	var sv = local [i];
}

• You can use foreach to simplify the code (single call)

foreach (var sv in v.Subviews)
	Console.WriteLine (sv);

That still has an extra cost (over for) but much lower than the above code doing multiple transitions.

Note that I'm not against adding a GetSubview method but it's more for the later case, i.e. getting the Count, without marshalling everything (for possibly nothing), e.g.

void ShowStatus (View v)
{
   label.Text = "{v.Subviews.Count} item" + v.Subviews.Count > 1 ? "s" : String.Empty;
}

would be quite bad.

[1] same for UIView or any NSArray exposed as properties or many case not involving Xamarin code;
[2] there was a gendarme rule about this (but had too many false positives iirc), I wonder if there's a roslyn analyzer now...

[Therzok]

Yup, it depends on the use-case. Having both NSArray and Array bindings would allow the user to choose the better trade-off (gc vs cpu per loop iteration).

[abock]

This improvement is absolutely needed across the entire API surface of XI/XM. By applying the same technique to CALayer.Sublayers I was able to realize an 11x performance improvement in rendering a custom layer-backed view while aggressively scrolling it. This allowed us to realize about a 3x improvement of render overall, allowing us to render at well beyond 60fps.

[spouliot]

Impressive! I'm glad the discussion helped :)

Now we cannot fix the existing API without breaking changes. The right approach would be to drop managed arrays and instead expose NSArray<T> (ideally) or NSArray properties. That can only be done on XAMCORE_4_0. Still that's the best approach since it cover all the usual loop cases.

It's a bit more complex than just properties since every API that accept managed arrays would also need to be updated to use NSArray

In theory we could expose additional NSArray variants but that means changing application code - but if you know about the issue (and can change your code) then you can easily get around the performance hit by either using a local variable or foreach. In all cases, except the for without a local, the allocation will happen only once and the performance should be identical.

[migueldeicaza]

This would be a source-code breaking change, so I do not support it on a XAMCORE_4 world.

As discussed on chat if you end up looping over all the values in the array, you will be faster using the marshaling we provide than going through NSArray wrappers to fetch the values. And in fact, for Aaron's use case, by hoisting the access to those arrays out of the loop, he got his performance back as he was using that idiom.

For the scenarios where this does not work, we should explore adding an additional helper method (side-by-side) that could provide access to that method, maybe on a separate namespace as an extension method, so you could do "using UIKit.NSArrayAPI" and then be able to call "NSArray UIKit.GetSubViews ()" method on it.

[Therzok]

I think it would be better, for discoverability, to have them as side-by-side methods which expose the NSArray, rather than an array. Having it in a different namespace does reduce pollution in the completion list by default, but I'd prefer discoverability over verbosity of a completion list (I think we won't have more than 4 matches in the list when writing Subview).

[Therzok]

Another thing to note is that something is busted when using NSArray<NSView> from the patch linked by @chamons.

Sometimes, when iterating the NSArray<NSView> via foreach, I'd get the following stacktrace:

Unhandled Exception:
  System.InvalidCastException: Unable to cast object of type 'Foundation.NSArray`1[[Foundation.NSString, Xamarin.Mac, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065]]' to type 'Foundation.NSArray`1[[AppKit.NSView, Xamarin.Mac, Version=0.0.0.0, Culture=neutral, PublicKeyToken=84e04ff9cfb79065]]'
    at ObjCRuntime.Runtime.GetNSObject[T] (System.IntPtr ptr) [0x00160] in /Library/Frameworks/Xamarin.Mac.framework/Versions/5.6.0.1/src/Xamarin.Mac/ObjCRuntime/Runtime.cs:1295
    at AppKit.CocoaExtensions.GetSubviews (AppKit.NSView view) [0x00012] in /Users/therzok/Work/md/vs-editor-core/src/Editor/Text/Impl/CocoaView/CocoaExtensions.cs:68

The solution was to use NSArray and use GetItem<NSView> on it.

[spouliot]

@Therzok please file a bug w/test case about the later - it should not throw.

[migueldeicaza]

This is an advanced feature - I rather document how to have an idiom for users of the advanced API than have to explain the difference in the base api.