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

Improve binding redirects performance #3239

Merged
merged 1 commit into from Jun 7, 2018

Conversation

Projects
None yet
5 participants
@mrinaldi
Contributor

mrinaldi commented Jun 6, 2018

This PR changes from Mono.Cecil to AssemblyReader to read the assemblies to find out if binding redirects is needed.

paket install results:

Project Size Before PR After PR
Small Solution (4 projects) 1 second 1 second
Medium Solution (29 projects) 26 seconds 4 seconds
Large Solution (72 projects) 1 minute, 33 seconds 13 seconds
@forki

This comment has been minimized.

Member

forki commented Jun 7, 2018

IIRC we used Cecil because of mono bugs. Maybe they are fixed now and we can try again

@forki forki requested review from vasily-kirichenko and matthid Jun 7, 2018

@matthid

This comment has been minimized.

Member

matthid commented Jun 7, 2018

Is this the same API as in
#3223?

Why do we need no new dependency here?

Is this reflection loading the assembly or just reading it?

/cc @viktor-svub

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Jun 7, 2018

seems similar to #3223 but it's very different in both intent and implementation:

improve binding redirects performance #3239 removing Mono.Cecil #3223
much cleaner / simpler refactoring bigger refactoring to different API
targets and really improves performance targets removal of 3rd-party dependency
does not remove the original dependency may improve performance as side-effect
hard-coded binary metadata reader standard ECMA-335 metadata reader
custom / 3rd-party / legacy? AssemblyReader.fs Microsoft System.Reflection.Metadata
built-in without other dependencies requires System.Collections.Immutable
paket-files\fsprojects\FSharp.TypeProviders.SDK NuGet, may need ILMerge on some platforms
unknown cross-platform compatibility net45, PCL, UAP, netstandard1.1, ...

I'd recommend to take #3239 (this), if/when/after known/proven safe on supported platforms,
and replace it by #3223 if applicable, under the same conditions, to move towards net/standard

@forki

This comment has been minimized.

Member

forki commented Jun 7, 2018

ok let's try it!

@forki forki merged commit d78c221 into fsprojects:master Jun 7, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@smoothdeveloper

This comment has been minimized.

Contributor

smoothdeveloper commented Jun 7, 2018

@viktor-svub wow that's a nice summary!

@mrinaldi I like the performance figures!

@mrinaldi mrinaldi deleted the mrinaldi:improve-binding-redirects-performance branch Jun 7, 2018

@mrinaldi

This comment has been minimized.

Contributor

mrinaldi commented Jun 7, 2018

This branch was lying around for quite some time waiting for tests to be promoted to a PR.
TBH, I didn't see the #3223 before or went through the PR list at all.
I did try #3223 , though and the performance figures as very similar to the ones on this PR.
In fact, the numbers are so similar that I had to double check if I was running the correct paket.exe.

Anyway, nice work @viktor-svub

@matthid

This comment has been minimized.

Member

matthid commented Jun 10, 2018

I'm not sure this was the correct approach: It seems this AssemblyReader.fs thing doesn't actually exist anymore: https://github.com/fsprojects/FSharp.TypeProviders.SDK/tree/master/src
We would need to reference the complete type provider sdk in order to stay up-to-date. Therefore I think we should switch to @viktor-svub approach and use the microsoft package (or update to full type provider sdk).

In any case we should indeed go the complete way and remove mono.cecil instead of having multiple dependencies.

@viktor-svub Is it possible for you to update #3223? Or what do you think?

@viktor-svub

This comment has been minimized.

Contributor

viktor-svub commented Jun 11, 2018

I can update #3223, but it may take few days to find the time :)

@mrinaldi

This comment has been minimized.

Contributor

mrinaldi commented Jun 11, 2018

I'm not sure what exactly @matthid is asking @viktor-svub to update. It might be this, but I'll point out anyway in case it's not.

AssemblyReader is used in a couple or places throughout the code base. In fact, this was the reason I used it instead of something else.

That said, I think we should aim to remove it completely in case we go with the microsoft package for the points @matthid already pointed out. @viktor-svub let me know if you need any help with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment