Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ImageSharp-762: Added methods to pre-seed AoT compiler on iOS #767

Merged
merged 13 commits into from
Nov 12, 2018
Merged

ImageSharp-762: Added methods to pre-seed AoT compiler on iOS #767

merged 13 commits into from
Nov 12, 2018

Conversation

dmanning23
Copy link
Contributor

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

These changes add a few methods to get around the problems described in #762.
Basically, the heavy use of generics in ImageSharp is too much to figure out for the ahead-of-time compiler used in Xamarin.iOS. When a user tries to call the SaveAsGif method on an iOS device, it has to spin up the JIT compiler to build the necessary code... however iOS is strictly AoT, so this is an illegal op and throws an exception.
All these changes do is add a few methods that can be used to pre-seed the necessary code in the AoT compiler on iOS for the OctreeQuantizer used in the SaveAsGif method. I saw a few more of these exceptions in the WuQuantizer and the dithering processors. If yall are ok with this recipe for fixing these errors, I can add a few more of these stubs.

Cheers!

@CLAassistant
Copy link

CLAassistant commented Nov 2, 2018

CLA assistant check
All committers have signed the CLA.

@JimBobSquarePants
Copy link
Member

Hi @dmanning23

I've been putting some thought to this and I think I've come up with a good class design that will let us incrementally add workarounds for AOT compiler limitations without negatively impact the public API.

This design allows you to call a single method at startup that will allow you to easily seed the compiler.

// Seed a single pixel format.
AotCompiler.Seed<Rgba32>();

// Seed two pixel formats.
AotCompiler.Seed<Rgba24, Rgba32>();

// Seed three pixel formats.
AotCompiler.Seed<Gray8, Rgba24, Rgba32>();

I'm assuming, I could be wrong of course, that you need to tell the compiler about each pixel format you intend to use. If not, great, we can simplify.
Here's a stubbed out class that I would like you to use to add your methods.

// Copyright (c) Six Labors and contributors.
// Licensed under the Apache License, Version 2.0.

using SixLabors.ImageSharp.PixelFormats;

namespace SixLabors.ImageSharp.Advanced
{
    /// <summary>
    /// Unlike traditional Mono/.NET, code on the iPhone is statically compiled ahead of time instead of being
    /// compiled on demand by a JIT compiler. This means there are a few limitations with respect to generics,
    /// these are caused because not every possible generic instantiation can be determined up front at compile time.
    /// The Aot Compiler is designed to overcome the limitations of this compiler.
    /// </summary>
    public static class AotCompiler
    {
        /// <summary>
        /// Seeds the compiler using the given pixel format.
        /// </summary>
        /// <typeparam name="TPixel">The pixel format.</typeparam>
        public static void Seed<TPixel>()
             where TPixel : struct, IPixel<TPixel>
        {
            // This is we actually call all the individual methods you need to seed.
            // TODO: Do the discovery work to figure out what works and what doesn't.
        }

        /// <summary>
        /// Seeds the compiler using the given pixel formats.
        /// </summary>
        /// <typeparam name="TPixel">The first pixel format.</typeparam>
        /// <typeparam name="TPixel2">The second pixel format.</typeparam>
        public static void Seed<TPixel, TPixel2>()
             where TPixel : struct, IPixel<TPixel>
             where TPixel2 : struct, IPixel<TPixel2>
        {
            Seed<TPixel>();
            Seed<TPixel2>();
        }

        /// <summary>
        /// Seeds the compiler using the given pixel formats.
        /// </summary>
        /// <typeparam name="TPixel">The first pixel format.</typeparam>
        /// <typeparam name="TPixel2">The second pixel format.</typeparam>
        /// <typeparam name="TPixel3">The third pixel format.</typeparam>
        public static void Seed<TPixel, TPixel2, TPixel3>()
             where TPixel : struct, IPixel<TPixel>
             where TPixel2 : struct, IPixel<TPixel2>
             where TPixel3 : struct, IPixel<TPixel3>
        {
            Seed<TPixel, TPixel2>();
            Seed<TPixel3>();
        }
    }
}

Do you think this is a sensible approach?

@dmanning23
Copy link
Contributor Author

Yeah, that makes perfect sense 👍
I'll update the PR.

@JimBobSquarePants
Copy link
Member

@dmanning23 Glad you agree, thanks for your help on this.

The build is failing just now due to trailing whitespace errors. I've got a pretty draconian set of rules in the repo that seem harsh but have kept us sane over the last few years. If you build locally in release you'll see the errors.

@dmanning23
Copy link
Contributor Author

naw I dig it. All yall using StyleCopAnalyzer to setup those rules? I might set it up on a few projects of my own ;)

@antonfirsov
Copy link
Member

@dmanning thanks for this contribution! If it works for you I would say it's fine.

One minor thing: I know there is no good way for testing this stuff without adding an AOT target in our CI, but I would feel more safe if there were unit tests which at least make sure these methods can be inoked without causing trouble. (Eg Exceptions)

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

I think this is OK for a merge now.

@JimBobSquarePants
Copy link
Member

@dmanning23 Is there anything else you would like to add, or does this cover it?

@antonfirsov
Copy link
Member

@JimBobSquarePants @dmanning23 just one other thing:

How about ˛renaming AotCompiler to AotCompilerTools? (More correct semantics I guess.)

@JimBobSquarePants
Copy link
Member

How about ˛renaming AotCompiler to AotCompilerTools? (More correct semantics I guess.)

YES! Much better 👍

@codecov
Copy link

codecov bot commented Nov 12, 2018

Codecov Report

Merging #767 into master will decrease coverage by <.01%.
The diff coverage is 76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #767      +/-   ##
==========================================
- Coverage   88.52%   88.52%   -0.01%     
==========================================
  Files        1003     1005       +2     
  Lines       42738    42763      +25     
  Branches     3150     3150              
==========================================
+ Hits        37834    37854      +20     
- Misses       4212     4218       +6     
+ Partials      692      691       -1
Impacted Files Coverage Δ
...ssors/Quantization/OctreeFrameQuantizer{TPixel}.cs 98.71% <100%> (ø) ⬆️
...ests/ImageSharp.Tests/Advanced/AotCompilerTests.cs 100% <100%> (ø)
...rocessors/Quantization/WuFrameQuantizer{TPixel}.cs 96.29% <100%> (+0.25%) ⬆️
src/ImageSharp/Advanced/AotCompilerTools.cs 71.42% <71.42%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b196a08...6558464. Read the comment docs.

@dmanning23
Copy link
Contributor Author

Aight, got that last change in to rename to AotCompilerTools.

@JimBobSquarePants Yeah, this is ready to merge in whenever yall are OK with it 👍

@JimBobSquarePants JimBobSquarePants merged commit 088bb2b into SixLabors:master Nov 12, 2018
@JimBobSquarePants
Copy link
Member

Merged and thanks you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants