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.

Sign up for GitHub

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

Jump to bottom

Optimize format string parsing #5121

Merged
merged 5 commits into from
Jun 7, 2018
Merged

Optimize format string parsing #5121

merged 5 commits into from
Jun 7, 2018

Conversation

vasily-kirichenko
Copy link
Contributor

@vasily-kirichenko vasily-kirichenko commented Jun 6, 2018 •
edited
Loading

This reduces completion time from 6 to 3 seconds on ProvidedTypes.fs in https://github.com/fsprojects/FSharp.TypeProviders.SDK/tree/master/tests project.

Before

image

image

After

image

@dsyme
Copy link
Contributor

dsyme commented Jun 7, 2018 •
edited
Loading

I'm sceptical.. :) This code will re-allcoate an unevaluated lazy value on every invocation (since property evaluation is on-demand in F#). From a quick code review I don't see how this might make things faster? And looking at the numbers above it seems like all the numbers have been cut be 1/3 and the proportion of time looks about the same

However I do agree that we should do the lines computation only once.

FWIW I don't like having Lazy<_> in APIs for this kind of reason, as it becomes harder to reason about lifetime. In the code, just make the property

abstract FormatStringCheckContext : FormatStringCheckContext option

and put the on-demand computation on the implementing side.

@vasily-kirichenko
Copy link
Contributor Author

@dsyme wow, thanks, I’m stupid :( I’ll store the lazy in a field.

@vasily-kirichenko
Copy link
Contributor Author

The property syntax in F# is too similar to let val declaration, with totally different semantics :(

@vasily-kirichenko
Copy link
Contributor Author

Fixed the lazy usage, but the completion and everything on that file is awfully slow

_1

@vasily-kirichenko
Copy link
Contributor Author

The above gif is a very happy case. There are ones when completion after asms. appears in 13 seconds.

@dsyme
Copy link
Contributor

dsyme commented Jun 7, 2018

Fixed the lazy usage, but the completion and everything on that file is awfully slow

Yes, it's a big horrible file. When #4579 gets fixed we should be able to ship the TPSDK as a DLL (and split that thing into multiple files).

dsyme
dsyme reviewed Jun 7, 2018
View reviewed changes
src/fsharp/NameResolution.fs Outdated
@@ -1521,6 +1525,19 @@ type TcResultsSinkImpl(g, ?source: string) =
let capturedOpenDeclarations = ResizeArray<OpenDeclaration>()
let allowedRange (m:range) = not m.IsSynthetic

let formatStringCheckContext =
lazy (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: no parens needed (though they don't hurt)

KevinRansom
KevinRansom approved these changes Jun 7, 2018
View reviewed changes
Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this.

@KevinRansom KevinRansom merged commit 5e8765f into dotnet:master Jun 7, 2018
dsyme pushed a commit that referenced this pull request Jun 8, 2018
@dotnet-bot @dsyme
Merge master to dev16.0 (#5134)
021c6e7 
* LOC CHECKIN | Microsoft/visualfsharp master | 20180607 (#5130)

* Optimize format string parsing (#5121)

* normalize source and create line endings array both used for format string parsing once per file

* comments

* let lazy do its job

* remove parens
@davkean davkean mentioned this pull request Nov 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsyme dsyme dsyme approved these changes

@KevinRansom KevinRansom KevinRansom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vasily-kirichenko @dsyme @KevinRansom