mapbox / mapbox-sdk-cs

C# libraries for Mapbox API layer
https://mapbox.github.io/mapbox-sdk-cs/
Other
20 stars 11 forks source link

Remove closures #62

Open david-rhodes opened 7 years ago

david-rhodes commented 7 years ago

From Map.cs:

foreach (CanonicalTileId id in cover)
{
    var tile = new T();

    Tile.Parameters param;
    param.Id = id;
    param.MapId = this.mapId;
    param.Fs = this.fs;

        // this is a closure!
    tile.Initialize(param, () => { this.NotifyNext(tile); });

    this.tiles.Add(tile);
    this.NotifyNext(tile);
}

As noted from the Unity documentation:

Currently, inspection of code generated by IL2CPP reveals that the simple declaration and assignment of a variable of type System.Function allocates a new object. This is true whether the variable is explicit (declared in a method/class) or implicit (declared as an argument to another method).

As such, any use of anonymous methods under the IL2CPP scripting backend allocates managed memory. This is not the case under the Mono scripting backend.

Furthermore, IL2CPP displays dramatically different levels of managed memory allocation depending on the way in which a method argument is declared. Closures, as expected, allocate the most memory per call.

Unintuitively, predefined methods allocate nearly as much memory as closures when passed as arguments under the IL2CPP scripting backend. Anonymous methods generate the least amount of transient garbage on the heap, by one or more orders of magnitude.

This would require us to remove Action as method arguments. Prefer C# events?

/cc @brnkhy @isiyu @BergWerkGIS

wilhelmberg commented 7 years ago

An alternative would be to make the anonymous methods non-anonymous. :smirk: Although this would increase LOC.

I think Actions are easier to handle: eg no need to unregister event handler (-=) when done.

david-rhodes commented 7 years ago

@BergWerkGIS I typically favor the readability of non-anonymous methods, anyway. However, Unity states:

Unintuitively, predefined methods allocate nearly as much memory as closures when passed as arguments under the IL2CPP scripting backend

My understanding was that this is a predefined method, and therefore allocates extra memory. I think the cost is in passing functions around.