Pass an FSharpProjectOptions down into the lint machinery when available#845
Pass an FSharpProjectOptions down into the lint machinery when available#845Numpsy wants to merge 1 commit intofsprojects:masterfrom
Conversation
| GlobalConfig: Rules.GlobalRuleConfig | ||
| TypeCheckResults: FSharpCheckFileResults option | ||
| ProjectCheckResults: FSharpCheckProjectResults option | ||
| ProjectOptions: Lazy<FSharpProjectOptions option> |
There was a problem hiding this comment.
This is Lazy beacuse if it's expensive to calculate and only used by a few rules then the work can be avoided in cases where those rules aren;t being run)
There was a problem hiding this comment.
Please these explanations have to go in the commit msg.
There was a problem hiding this comment.
commit message updated
46314f3 to
fdb83e4
Compare
|
@webwarrior-ws please review |
|
LGTM |
|
@Numpsy why draft? is it because of the red CI? |
|
I'm not sure what's going on with the CI, it looks like the last nightly build failed with the same error? |
The nightly builds seem to have started working again now, but I don't seem to have the ability to rerun the failed build here without pushing more changes |
|
As we briefly discussed in #846, this PR is great, makes things way faster, and I'd love to see it merged! A lot of AI stuff so if you're fearful of slop feel free to ignore this or at least take it with a grain of salt, although this comment at least was written with my ten fleshy fingers. The Pathological Case: Projects with lots of local dependent packages, e.g. FsHotWatch are particularly slow, and this PR makes them much faster. I (claude actually) built a FSharpLint -> Analyzer shim to run FSharp linters via Ionide.Analyzer. Then I benchmarked it vs the fsharplint CLI. Small projects are faster with FsharpLint CLI but pathological cases with lots of local dependencies are way faster running the same FSharpLinters via my shim when launched through the Ionide.Analyzer CLI. I tried to figure out what was different. One obvious difference was sharing a FSharpChecker & MSBuild per-solution instead of instantiating one for each sub project. So I asked claude to implement those changes (on top of this branch) and benchmark them...
|
|
There may or may not be other perf changes if we get #850 done as Microsoft have been trying to speed it up, but with the odd regression as well. I'll try updating the branch to make the CI run again now that the nightly build seems to be fixed |
Rather than reading it from FSharpCheckProjectResults on every use, which appears to have a substantional performance cost. The project options is stored as a Lazy because it's expensive to calculate and only used by a few rules, so the work can be avoided altogether if those rules aren't being run. refs fsprojects#770.
fdb83e4 to
c7ed077
Compare
Rather than reading it from FSharpCheckProjectResults on every use, which appears to have a substantional performance cost.
refs #770 (specifically #770 (comment))
This is an attempt at making the FSharpProjectOptions directly available through the lint options and then constructing it once rather than on every call which seems to have a substantional performance impact.
This is more like how its done in the F# Analyzers SDK, where the project check results and the project options are both passed down from the top.
I'm also reminded when looking that they also have a
ProjectFileNameproperty on the top level options so that consumers don't need to worry about where it came from. That could potentially be done here given that all consumers as it stands only want the file name, though having the whole options available is more extensible for the future.This change does seem to have a positive performance impact with the current code using FCS 43.9 as well as removing the large regression when using 43.10 -
Build of just this change - https://github.com/Numpsy/FSharpLint/actions/runs/22036364219 (this takes a couple of minutes off the runtime of the self-lint step in the CI runs as compared to the current master)
Build using this change and FCS 43.10.103 - https://github.com/Numpsy/FSharpLint/actions/runs/21925277621