-
Notifications
You must be signed in to change notification settings - Fork 790
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
[WIP, Investigation] Cache project options #2801
[WIP, Investigation] Cache project options #2801
Conversation
How does this work on the dense solution? |
@saul
|
Looks like That's after I've made the cache here https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/vs/IncrementalBuild.fs#L1671 global, like static let cache = TimeStampCache(defaultTimeStamp) (it's obviously must be invalidated on file changes (watcher?), but it's not the point here) |
That looks a lot better, thanks for investigating this. I think AreSameForChecking doesn't need to be recursive, though. Maybe remove the recursion and see what the performance is like? |
@saul With no recursion it was loading CPU at 15% for about 4 minutes, then dropped to zero. I'm not sure we can remove recursion there because if a referenced project changed, we won't know that and project options becomes outdated. |
There's surely got to be a better way of doing these comparisons (e.g. making the project options type abstract and giving the values a unique stamp, or a SHA1 hash, then just comparing stamps/hashes) |
Any closing thoughts @vasily-kirichenko ? |
@saul no. I think I'm not going to touch project system in near future. Let's wait until MS port it to CPS (no sarcasm, I'm just tired with this stuff). |
Fair enough - as this is in the language service though I don't think CPS will help us here. |
It caches
FSharpProjectOptions
and it won't work if projects changed. The log shows the following:So, yes, there is lot redundant work, but all these are done in about 2 seconds, so...