-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
WIP: Use FastExpressionCompiler for compilation and diagnostics #4446
base: master
Are you sure you want to change the base?
Conversation
@@ -53,5 +53,6 @@ | |||
<Using Include="AutoMapper.Execution.ExpressionBuilder" Static="true" /> | |||
<Using Include="AutoMapper.Execution" /> | |||
<Using Include="AutoMapper.Configuration" /> | |||
<Using Include="FastExpressionCompiler" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my view, the best solution would be just a hook to allow changing the compiler. So AM doesn't need to reference FEC. We would advertise the change in the upgrade guide and we could link to an unit test in this repo that shows it in action. Perhaps an extension method to IMapperConfiguration
to simplify the usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lbargaoanu
Yeah, I was thinking about something like moving the CompileFast
to the delegate.
It may look something like this:
new MapperConfiguration(cfg => cfg.UseCompiler(expr => expr.CompileFast()));
I am not familiar with the details of AM configuration, but it would be good to set Compiler
globally, e.g. statically.
IMHO, this option does not require the granularity anyway.
This way, I may just add another UnitTests project, bring the FEC dependency into it, reference the source UnitTests and configure the FEC statically. Then run all the tests for the regression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, smth like that. I don't know about running the tests with FEC here, in this repo. What would we do when smth breaks, disable the offending test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lbargaoanu Yes, Ignore and issue in my repo. This will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should start with a small PR that just introduces the hook in the internal API. The test can simply use the built in compiler and assert that it's called. We can add FEC afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree, let's start small. Unfortunately I think the only way to "know" that FEC works is to run against all the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I think it's kind of weird to have the tests here when the fix is in another repo :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may run the tests on my side as well via devcontainer or similar, via checking out AutoMapper, copying my own project into it, referencing AM tests, configuring AM with FEC and running the tests. Likely, it will work too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it helps, we could have a hook in our tests to allow changing the compiler when running the tests, maybe with an environment variable. Just a thought, maybe there are better ways.
Hi @jbogard,
Time goes, and I was able to improve the FastExpressionCompiler enough to pass AutoMapper unit tests.
This PR is just a demonstration. The minimal FEC files are included directly.
Today, in addition to compilation speed and some invocation speed, FEC has capabilities to inspect and output Expressions and C# as demonstrated in
MapperConfiguration
.The sources of FEC used here correspond to FEC v4.2.0
If it is an interest to you, I may add
MapperConfiguration
hook, so the consumer may switch to the FEC if he wants to.Anyway, it was fun for me to make things work :-)
Cheers!