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

[CompilerPerf] Faster name resolution #1514

Merged
merged 11 commits into from Oct 5, 2016

Conversation

@forki
Copy link
Contributor

commented Sep 2, 2016

Inspired by #1512 I optimized similar cases. This should bring faster name resolution

@KevinRansom

This comment has been minimized.

Copy link
Member

commented Sep 2, 2016

@dotnet-bot tet this please

@forki forki changed the title Faster name resolution + more predictions WIP: Faster name resolution + more predictions Sep 2, 2016

@forki forki force-pushed the forki:faster-nameres branch from 760d1f5 to 075d563 Sep 3, 2016

@forki forki changed the title WIP: Faster name resolution + more predictions Faster name resolution + more predictions Sep 3, 2016

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2016

ready for review/merge

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2016

FSharp.Core.Unittests.FSharp_Core.Microsoft_FSharp_Control.AsyncModule.AwaitWaitHandle.Timeout seems unrelated!? @eiriktsarpalis do you know this thingy?

@forki forki force-pushed the forki:faster-nameres branch 2 times, most recently from 9125789 to c1e4faa Sep 3, 2016

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2016

Ok I tried to test this. I wrote a batch file that runs fsc.exe in a loop on paket.core.
I changed the compiler to stop after type checking. My results are:

For 20 times compiling (stopping after type check) I see:

  • on master; 113s
  • after applying #1514: 102s
@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2016

It'd be interesting to see numbers for the whole pipeline, including creating binaries.

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2016

So other words: we would save about 0.5s (about 5%) for a Paket.Core compile with a
relatively small change. I find that pretty cool.

Am 03.09.2016 4:31 nachm. schrieb "Vasily Kirichenko" <
notifications@github.com>:

It'd be interesting to see numbers for the whole pipeline, including
creating binaries.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1514 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNLdJEpsvFHZ-m8IZnF0-bSowO60nks5qmYSugaJpZM4Jz5qL
.

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2016

@forki I cannot reporduce your numbers on compiling Paket.Core. I have 8:38..8:40 on FSC master and 8:48..8:50 on your branch.

"d:\github\visualfsharp\release\net40\bin\fsc.exe" -o:obj\Release\Paket.Core.dll -g --debug:pdbonly --noframework --define:TRACE --doc:....\bin\Paket.Core.XML --optimize+ -r:D:\github\Paket\packages\Chessie\lib\net40\Chessie.dll -r:D:\github\Paket\packages\FSharp.Core\lib\net40\FSharp.Core.dll -r:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework.NETFramework\v4.5\mscorlib.dll" -r:D:\github\Paket\packages\Newtonsoft.Json\lib\net45\Newtonsoft.Json.dll -r:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework.NETFramework\v4.5\System.Core.dll" -r:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework.NETFramework\v4.5\System.Data.dll" -r:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework.NETFramework\v4.5\System.Data.Linq.dll" -r:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework.NETFramework\v4.5\System.dll" -r:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework.NETFramework\v4.5\System.IO.Compression.dll" -r:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework.NETFramework\v4.5\System.IO.Compression.FileSystem.dll" -r:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework.NETFramework\v4.5\System.Numerics.dll" -r:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework.NETFramework\v4.5\System.Security.dll" -r:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework.NETFramework\v4.5\System.Xml.dll" -r:"C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework.NETFramework\v4.5\System.Xml.Linq.dll" --target:library --warn:3 --warnaserror:76 --vserrors --LCID:1033 --utf8output --fullpaths --flaterrors --subsystemversion:6.00 --highentropyva+ --sqmsessionguid:2e049b54-00aa-46a6-9620-996101adfb80 "C:\Users\kot\AppData\Local\Temp.NETFramework,Version=v4.5.AssemblyAttributes.fs" ....\paket-files\fsharp\FSharp.Data\src\CommonProviderImplementation\AssemblyReader.fs ....\paket-files\fsharp\FAKE\src\app\FakeLib\Globbing\Globbing.fs Async.fs AssemblyInfo.fs CustomAssemblyInfo.fs Domain.fs Constants.fs SemVer.fs VersionRange.fs Logging.fsi Logging.fs Xml.fs Utils.fs ConfigFile.fs PackageSources.fs FrameworkHandling.fs PlatformMatching.fs Requirements.fs ModuleResolver.fs RemoteDownload.fs RemoteUpload.fs PackageResolver.fs Nuspec.fs InstallModel.fs ReferencesFile.fs ProjectFile.fs SolutionFile.fs Nuget.fs NuGetV3.fs NuGetV2.fs DependenciesFile.fs DependencyModel.fs LockFile.fs DependencyChangeDetection.fs RestoreProcess.fs BindingRedirects.fs TemplateFile.fs NupkgWriter.fs ProcessOptions.fs PackagesConfigFile.fs InstallProcess.fs UpdateProcess.fs RemoveProcess.fs AddProcess.fs PackageMetaData.fs PackageProcess.fs Environment.fs Releases.fs Simplifier.fs VSIntegration.fs NugetConvert.fs FindOutdated.fs FindReferences.fs PublicAPI.fs

@7sharp9

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2016

What about compiling the compiler, how much does that save?

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2016

So in other words: your test says I made it slower?

Am 03.09.2016 21:14 schrieb "Dave Thomas" notifications@github.com:

What about compiling the compiler, how much does that save?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1514 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNPOCGXJeEvELcCPVVkrW3W-refyfks5qmcchgaJpZM4Jz5qL
.

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2016

@forki yes
@7sharp9 if you give me a command line args for fsc, I will run it :) It's not that easy these days since everything is done via .net core magic, I could not find anything useful in std out compiling the compiler inside VS.

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

10 full compile runs of Paket.Core:

  • on master: average 10.6s
  • on this PR: 10.2s

but it has lot of variance so I think this not a good metric

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

@7sharp9 same here no idea how to call fsc.exe to compile fsc.exe - this is now fully hidden in magic

@forki forki changed the title Faster name resolution + more predictions Faster name resolution Sep 4, 2016

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2016

What about a ps/fake script which:

  • git clone / fetch ~10 large F# GitHub repos
  • paket restore for all of them
  • compiles each of them 5 times in a row with a given fsc.exe + all the surrounding assemblies
  • creates a report with timings
  • iterate all the steps for a given set of folders where the compiler and stuff lay
@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

yes that sounds like good idea. I will try to write that today. Which projects could we use?

@forki forki closed this Sep 4, 2016

@forki forki reopened this Sep 4, 2016

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2016

Paket, FAKE, all of mbrace repos, fsharp.data, fsharpx.xxx, FCS (if it's possible to build it without DNC magic), F# Lint, all the popular TPs, like SqlClient, Swagger, AzureStorage. I think it's not very important what you choose.

@7sharp9

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2016

So build.cmd is now obsolete, having an easily buildable compiler is the most important thing.

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

@7sharp9 what?

@7sharp9

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2016

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

image

the 1bf329f is this PR

So there is improvement.

Next step: make FSharpPerf run on a CI.

@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2016

great stuff

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2016

with --typecheckonly flag on appveyor (see https://github.com/fsprojects/FSharpPerf/pull/5/files):

Compiler: 71c8798e19d6e15d3e6a98c80da658aa5ed2c630
   Project: Paket 4752.0ms
   Project: FSharpx.Collections 1410.0ms
   Project: FSharp.Data 2180.0ms
   Project: SQLProvider 3616.0ms
   Average Run: 11958.0ms
Compiler: 1bf329fa06b7e2e4d4ceab545b0e059e72be3e1c
   Project: Paket 4562.0ms
   Project: FSharpx.Collections 1402.0ms
   Project: FSharp.Data 2070.0ms
   Project: SQLProvider 3490.0ms
   Average Run: 11526.0ms
@vasily-kirichenko

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2016

It would be great if shows improvement in percents, like this:

Compiler: 71c8798e19d6e15d3e6a98c80da658aa5ed2c630
   Project: Paket 4752.0ms
   Project: FSharpx.Collections 1410.0ms
   Project: FSharp.Data 2180.0ms
   Project: SQLProvider 3616.0ms
   Average Run: 11958.0ms
Compiler: 1bf329fa06b7e2e4d4ceab545b0e059e72be3e1c
   Project: Paket 4562.0ms (-5.1%)
   Project: FSharpx.Collections 1402.0ms (-5.0%)
   Project: FSharp.Data 2070.0ms (-4.7%)
   Project: SQLProvider 3490.0ms (+1.1%) <<< !!!
   Average Run: 11526.0ms (-3.8%)
// Lookup: datatype constructors take precedence
match unionCaseSearch with
| Some ucase ->
success(resInfo,Item.UnionCase(ucase,false),rest)
| None ->
let isLookUpExpr = match lookupKind with LookupKind.Expr -> true | _ -> false

This comment has been minimized.

Copy link
@KevinRansom
@KevinRansom

This comment has been minimized.

Copy link
Member

commented Sep 19, 2016

@forki @dsyme There is a lot of churn here and not just whitespace ... but I would like to take this PR. It looks good to me.

@forki ... @dsyme Please let me know whether you agree ...

Kevin

@forki forki force-pushed the forki:faster-nameres branch from 1bf329f to 868c516 Sep 20, 2016

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2016

@KevinRansom I think it should be merged together with #1512 since that one fixes the original bug.

// Lookup: datatype constructors take precedence
match unionCaseSearch with
| Some ucase ->
success(resInfo,Item.UnionCase(ucase,false),rest)
| None ->
let isLookUpExpr = lookupKind = LookupKind.Expr

This comment has been minimized.

Copy link
@dsyme

dsyme Sep 20, 2016

Collaborator

Please use parentheses here

let isLookUpExpr = (lookupKind = LookupKind.Expr)
@dsyme

This comment has been minimized.

Copy link
Collaborator

commented Sep 20, 2016

Looks like nuget package restore is failing?

Errors in packages.config projects
    Unable to find version '14.0.24516-Pre' of package 'VisualCppTools'.
      C:\Users\dotnet-bot\.nuget\packages\: Package 'VisualCppTools.14.0.24516-Pre' is not found on source 'C:\Users\dotnet-bot\.nuget\packages\'.
      C:\Users\dotnet-bot\AppData\Local\NuGet\Cache: Package 'VisualCppTools.14.0.24516-Pre' is not found on source 'C:\Users\dotnet-bot\AppData\Local\NuGet\Cache'.
      https://vcppdogfooding.azurewebsites.net/nuget/: Package 'VisualCppTools.14.0.24516-Pre' is not found on source 'https://vcppdogfooding.azurewebsites.net/nuget/'.
      https://dotnet.myget.org/F/dotnet-buildtools/api/v3/index.json: Package 'VisualCppTools.14.0.24516-Pre' is not found on source 'https://dotnet.myget.org/F/dotnet-buildtools/api/v3/index.json'.
      https://dotnet.myget.org/F/dotnet-core/api/v3/index.json: Package 'VisualCppTools.14.0.24516-Pre' is not found on source 'https://dotnet.myget.org/F/dotnet-core/api/v3/index.json'.
      https://api.nuget.org/v3/index.json: Package 'VisualCppTools.14.0.24516-Pre' is not found on source 'https://api.nuget.org/v3/index.json'.
      https://www.myget.org/F/fsharp-daily/api/v3/index.json: Package 'VisualCppTools.14.0.24516-Pre' is not found on source 'https://www.myget.org/F/fsharp-daily/api/v3/index.json'.
@dsyme

This comment has been minimized.

Copy link
Collaborator

commented Sep 20, 2016

I took a look at the code and it all looks good.

@dsyme

This comment has been minimized.

Copy link
Collaborator

commented Sep 20, 2016

@forki I'm a little sceptical about the use of AppVeyor timings - do you have any timings from a dedicated machine? Or do we know if our AppVeyor runs are on the same dedicated, real, stable, physical, not-virtual, not-being-used-for-anything-else machine?

@dsyme dsyme changed the title Faster name resolution [CompilerPerf] Faster name resolution Sep 20, 2016

@forki

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2016

Tbh I'm sceptical about the whole approach.
But one thing is sure: you can't compare different runs against each other.
The thing we do with appveyor is that we do 2 runs in one build against the
same vm. That's OK. But if you rerun you get new vm

Am 20.09.2016 19:34 schrieb "Don Syme" notifications@github.com:

@forki https://github.com/forki I'm a little sceptical about the use of
AppVeyor timings - do you have any timings from a dedicated machine? Or do
we know if our AppVeyor runs are on a dedicated machine?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1514 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AADgNF1_saYlBDKZY-VRpVHKZqteFDVUks5qsBk6gaJpZM4Jz5qL
.

@KevinRansom

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

@dotnet-bot test this please

@KevinRansom KevinRansom merged commit 0c622b4 into dotnet:master Oct 5, 2016

4 checks passed

Windows_NT Debug Build Build finished.
Details
Windows_NT Release_ci_part1 Build Build finished.
Details
Windows_NT Release_ci_part2 Build Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.