Skip to content

Fix ProjectCracker, optimize Filename.checkPathForIllegalChars #659

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

Merged
merged 5 commits into from
Oct 25, 2016

Conversation

vasily-kirichenko
Copy link
Contributor

This function popped up in a profiler as one of hot paths. With this PR it disappears.

Path.GetInvalidPathChars() returns 36 chars and checking, say, 200 chars path, means 7200 array accesses and char comparisons. It seems this function is called very often.

fixed: ProjectCracker returns unnormalized (regarding to platform) ProjectFileName list, which causes symbols to be not equal while Find Usages performs
@vasily-kirichenko vasily-kirichenko changed the title memoize Filename.checkPathForIllegalChars Fix ProjectCracker, optimize Filename.checkPathForIllegalChars Oct 23, 2016
@dsyme
Copy link
Contributor

dsyme commented Oct 24, 2016

CI is failing because on .NET Core 1.0 we have

error FS0039: The field, constructor or member 'InvariantCultureIgnoreCase' is not defined

@vasily-kirichenko
Copy link
Contributor Author

@dsyme Thanks!

@dsyme
Copy link
Contributor

dsyme commented Oct 24, 2016

Hmmm On both Travis and AppVeyor:

Errors and Failures:
1) Test Failure : FSharp.Compiler.Service.Tests.ExprTests.Check use of type provider that provides calls to F# code
     Expected: 0
Actual: 3
  Expected: 0
  But was:  3

at FsUnit.shouldEqual[a] (a x, a y) <0x40e11900 + 0x0018b> in <filename unknown>:0
at FSharp.Compiler.Service.Tests.ExprTests.Check use of type provider that provides calls to F# code () <0x40324800 + 0x001c8> in <filename unknown>:0

@vasily-kirichenko
Copy link
Contributor Author

Yeah, same failure on my machine...

@vasily-kirichenko
Copy link
Contributor Author

So, ProjectFileNames and OtherOptions are in a mess. Before this PR ProjectFileNames was always empty and files names were in OtherOptions. And this worked somehow. I thought the proper way was to copy files names to ProjectFileNames from OtherOptions. But it messed things up since some code combines files names from those two lists. So, in the last commit I removed file names from OtherOptions, but it seems some other code uses OtherOptions and ignores ProjectFileNames :(

@vasily-kirichenko
Copy link
Contributor Author

OK, I fixed the tests according to this behavior:

  • source files go to ProjectFileNames only

@dsyme dsyme merged commit 9893d33 into fsharp:master Oct 25, 2016
@dsyme
Copy link
Contributor

dsyme commented Oct 25, 2016

@vasily-kirichenko We should really merge the perf fix into Microsoft\visuafsharp too

@dsyme
Copy link
Contributor

dsyme commented Oct 25, 2016

@vasily-kirichenko Thanks for the contribution!

@vasily-kirichenko
Copy link
Contributor Author

@dsyme I ported the perf fix to visualfsharp repo and tested it on FCS project. Absolutely no improvements: 43 seconds on F# 4.0/4.1(master)/this perf fix. So I don't think it's worth to make this change in VFT.

@dsyme
Copy link
Contributor

dsyme commented Oct 26, 2016

@vasily-kirichenko It might be that the fix applies primarily in the IDE tools - endless rechecking of fie names while preparing project options or doing incremental building. So perhaps the fix would apply specifically to the Visual F# IDE Tools. Anyway, it's ok - we can back port at some point, or perhaps we eventually work out how to merge repos.

@vasily-kirichenko
Copy link
Contributor Author

Ah, I see. It's very easy to port it, so I'll make a PR in a minute or two.

@vasily-kirichenko
Copy link
Contributor Author

dotnet/fsharp#1662

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants