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

Try to skip the security check #1356

Merged
merged 4 commits into from Jun 23, 2016
Merged

Try to skip the security check #1356

merged 4 commits into from Jun 23, 2016

Conversation

lbargaoanu
Copy link
Member

I got about 40%. Before and after.

@lbargaoanu
Copy link
Member Author

It still shows a lot of clr.dll, so maybe I missed something. When I tested at home in a simpler case, the difference was clear, now I'm not so sure :)

@lbargaoanu
Copy link
Member Author

The build fails. Apparently I can't get the #if right. I cannot test locally because the .NET Core project doesn't build:

1>C:\Program Files (x86)\MSBuild\Microsoft\VisualStudio\v14.0\DotNet\Microsoft.DotNet.Common.Targets(241,5): error : Object reference not set to an instance of an object.

@TylerCarlson1
Copy link
Member

Looking at the benchmarks in AppVoyer it looks about the same if not worse :(.
I measure performance by comparing Flattening mapper vs hard coded time taken to execute.
You build has a ratio of 47x where mine is about 13.5x.
Latest accepted PR got about 37x in AppVoyer.
Then I check the other ones, but usually their performance should either all go up or down when you are just changing compile method.

I don't know if it has something to do with the timer change or what.

@lbargaoanu
Copy link
Member Author

It's not easy to tell what happens in more detail because symbols for clr.dll are nowhere to be found. The test as it is in the Benchmark doesn't have enough samples for the Flattening mapper. I tested with 10 times as many. The improvement is there to see in the profiler results and the timings for the benchmark. What exactly you did in your PR, I don't know :) This tries to handle just the security check I was talking about. And it does, I'm just not sure it removes it completely because the symbols are missing from the MS servers.

@lbargaoanu
Copy link
Member Author

Well, I ran on a different machine where I was able to get the symbols for clr.dll and the check is nowhere to be found. Apparently it's the JIT. Reasonably sure this works as expected.

@lbargaoanu
Copy link
Member Author

@jbogard @TylerCarlson1 About .NET Core not building. Is it me? :)

@TylerCarlson1
Copy link
Member

@lbargaoanu I get that sometimes and sometimes it gives me a file and I have to delete the targets or something. I don't remember the specifics but you have to edit a file that's not part of git.

Also one concern with the PR is because the HighPerformanceTimer stops working, will that have ill effects on other people's code who use it? Also does that mean they have to manualy set assembly types, or will the nuget do that for them? Kind of like how Automapper 3.x had to add Automapper.Net4 to the solution's source files.

@lbargaoanu
Copy link
Member Author

Thanks, I guess I'll see :) Of course they would be in the same situation. That's why I was saying that this must be a bug at some level. I'm working around something that should work as is. The system assemblies have two of the attributes, but they are marked SecurityCritical which makes sense for them but not for AutoMapper I would say. Maybe there is a better mix :)

@jbogard jbogard added this to the 5.0.0 milestone Jun 23, 2016
@jbogard jbogard merged commit 59ed7d9 into AutoMapper:master Jun 23, 2016
@lbargaoanu lbargaoanu deleted the SecurityCheck branch June 24, 2016 07:34
@lock
Copy link

lock bot commented May 6, 2019

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

@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants