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

Fast expression compiler #3103

Closed
wants to merge 14 commits into from
Closed

Fast expression compiler #3103

wants to merge 14 commits into from

Conversation

jbogard
Copy link
Member

@jbogard jbogard commented May 29, 2019

The tests fail though.

@lbargaoanu
Copy link
Member

A lot of tests fail.

@jbogard
Copy link
Member Author

jbogard commented May 29, 2019

Looks like we're getting a lot of System.InvalidProgramException : Common Language Runtime detected an invalid program. @dadhi any ideas of how I should debug this?

@lbargaoanu
Copy link
Member

lbargaoanu commented May 29, 2019

It's possible to see the generated IL in VS using an extension. But generating the right IL, not so simple :)

@dadhi
Copy link

dadhi commented May 29, 2019

@jbogard, first identify what tests are failing. Try to isolate and minimize the expressions used in these tests. Test them with FEC.

I will look when I have time. I remember, that I had a fork with FEC for MediatR maybe a 2 years old. The tests were passing at the end.

@jbogard
Copy link
Member Author

jbogard commented May 29, 2019

@dadhi lol did you? I guess I do have a bit of Activator.CreateInstance in there.

@jbogard
Copy link
Member Author

jbogard commented May 29, 2019

@dadhi I made a very simple mapping to show the problem.

Here's the test:

using Shouldly;
using Xunit;

namespace AutoMapper.UnitTests.Bug
{
    public class FastExpressionCompilerBug
    {
        public class Source
        {
            public int Value { get; set; }
        }

        public class Dest
        {
            public int Value { get; set; }
        }

        [Fact]
        public void ShouldWork()
        {
            var config = new MapperConfiguration(cfg => cfg.CreateMap<Source, Dest>());
            var mapper = config.CreateMapper();
            var expression = mapper.ConfigurationProvider.BuildExecutionPlan(typeof(Source), typeof(Dest));

            var source = new Source {Value = 5};
            var dest = mapper.Map<Dest>(source);

            dest.Value.ShouldBe(source.Value);
        }
    }
}

And here's the expression (from ReadableExpressions.Visualizer):

(src, dest, ctxt) =>
{
    FastExpressionCompilerBug.Dest typeMapDestination;
    return (src == null)
        ? null
        : {
            typeMapDestination = dest ?? new FastExpressionCompilerBug.Dest();
            try
            {
                var resolvedValue = ((src == null) || false) ? default(int) : src.Value;
                typeMapDestination.Value = resolvedValue;
            }
            catch (Exception ex)
            {
                throw new AutoMapperMappingException(
                    "Error mapping types.",
                    ex,
                    AutoMapper.TypePair,
                    TypeMap,
                    PropertyMap);

                return default(int);
            }

            return typeMapDestination;
        };
}

@jbogard
Copy link
Member Author

jbogard commented May 29, 2019

And here's the debug view:

.Lambda #Lambda1<System.Func`4[AutoMapper.UnitTests.Bug.FastExpressionCompilerBug+Source,AutoMapper.UnitTests.Bug.FastExpressionCompilerBug+Dest,AutoMapper.ResolutionContext,AutoMapper.UnitTests.Bug.FastExpressionCompilerBug+Dest]>(
    AutoMapper.UnitTests.Bug.FastExpressionCompilerBug+Source $src,
    AutoMapper.UnitTests.Bug.FastExpressionCompilerBug+Dest $dest,
    AutoMapper.ResolutionContext $ctxt) {
    .Block(AutoMapper.UnitTests.Bug.FastExpressionCompilerBug+Dest $typeMapDestination) {
        .If ($src == null) {
            .Default(AutoMapper.UnitTests.Bug.FastExpressionCompilerBug+Dest)
        } .Else {
            .Block() {
                $typeMapDestination = ($dest ?? .New AutoMapper.UnitTests.Bug.FastExpressionCompilerBug+Dest());
                .Try {
                    .Block(System.Int32 $resolvedValue) {
                        .Block() {
                            $resolvedValue = .If (
                                $src == null || False
                            ) {
                                .Default(System.Int32)
                            } .Else {
                                $src.Value
                            };
                            $typeMapDestination.Value = $resolvedValue
                        }
                    }
                } .Catch (System.Exception $ex) {
                    .Block() {
                        .Throw .New AutoMapper.AutoMapperMappingException(
                            "Error mapping types.",
                            $ex,
                            .Constant<AutoMapper.TypePair>(AutoMapper.TypePair),
                            .Constant<AutoMapper.TypeMap>(AutoMapper.TypeMap),
                            .Constant<AutoMapper.PropertyMap>(AutoMapper.PropertyMap));
                        .Default(System.Int32)
                    }
                };
                $typeMapDestination
            }
        }
    }
}

@jbogard
Copy link
Member Author

jbogard commented May 31, 2019

See also: dadhi/FastExpressionCompiler#197

@dadhi
Copy link

dadhi commented Jun 20, 2019

Hi @jbogard

The cases reported in dadhi/FastExpressionCompiler#196 and dadhi/FastExpressionCompiler#197
are fixed.

Here is the clue about performance (the latest FEC master) using the expression from dadhi/FastExpressionCompiler#196

                                     Method |       Mean |     Error |    StdDev | Ratio | RatioSD | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
------------------------------------------- |-----------:|----------:|----------:|------:|--------:|------------:|------------:|------------:|--------------------:|
                                    Compile | 254.985 us | 3.9906 us | 3.7328 us | 28.47 |    0.56 |      1.9531 |      0.9766 |           - |            10.93 KB |
                                CompileFast |  11.032 us | 0.0558 us | 0.0522 us |  1.23 |    0.01 |      0.6256 |      0.3052 |      0.0305 |             2.91 KB |
                CompileFast_LightExpression |   8.961 us | 0.0921 us | 0.0769 us |  1.00 |    0.00 |      0.6256 |      0.3052 |      0.0305 |             2.91 KB |
                 CompileFast_WithoutClosure |   9.530 us | 0.0917 us | 0.0858 us |  1.06 |    0.01 |      0.6256 |      0.3052 |      0.0305 |             2.88 KB |
 CompileFast_LightExpression_WithoutClosure |   8.024 us | 0.0447 us | 0.0349 us |  0.90 |    0.01 |      0.6256 |      0.3052 |      0.0305 |             2.88 KB |

@jbogard
Copy link
Member Author

jbogard commented Jun 21, 2019

@dadhi Nice!

@jbogard jbogard force-pushed the FastExpressionCompiler branch from dbffcf7 to 3447e31 Compare June 21, 2019 13:02
@jbogard
Copy link
Member Author

jbogard commented Dec 18, 2019

Yikes. Still tons of test failures:

Test Run Failed.
Total tests: 1493
     Passed: 465
     Failed: 1028
 Total time: 37.4304 Seconds

@dadhi
Copy link

dadhi commented Dec 19, 2019

Sad..
I had started some work in my fork https://github.com/dadhi/AutoMapper/tree/FECv3

The problem is that expressions are complex and composed in a different places, so I need ToCodeString them.. but this is currently available for LightExpression - and I've started switching AM to LE. Then I immediately stuck on absence of ExpressionVisitor - now adding it...

@jbogard
Copy link
Member Author

jbogard commented Jan 5, 2022

Meh.

@jbogard jbogard closed this Jan 5, 2022
@lbargaoanu lbargaoanu deleted the FastExpressionCompiler branch January 16, 2022 05:27
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants