Skip to content

Conversation

DedSec256
Copy link
Contributor

@DedSec256 DedSec256 commented Mar 17, 2022

We need to be able to quickly get project/script options from the FCS cache.
This PR adds GetCachedProjectOptions(projectOrScriptFileName, isScript) method.

if incrementalBuildersCache.ContainsSimilarKey (AnyCallerThread, options) then
parseCacheLock.AcquireLock(fun ltok ->
for sourceFile in options.SourceFiles do
checkFileInProjectCache.RemoveAnySimilar(ltok, (sourceFile, 0L, options))
Copy link
Member

Choose a reason for hiding this comment

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

Hm, it seems this will also erase cached results in "similar" projects, e.g. there could be cached results for a file in another target framework in the same project. This API shouldn't touch other "similar" projects and only remove cache for the "same" project options, though this shouldn't likely be a real problem in practice. @DedSec256 could you take a look, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getOrCreateBuilder function does similar things and uses RemoveAnySimilar too https://github.com/dotnet/fsharp/blob/main/src/fsharp/service/service.fs#L419

Copy link
Member

Choose a reason for hiding this comment

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

That's interesting. We could probably change it there too then, but it also shows it's probably not that bad to do it like that this time, since this approach is already used in other places and we haven't noticed it before.

@dsyme
Copy link
Contributor

dsyme commented Mar 18, 2022

We need to be able to quickly get project/script options from the FCS cache

Could you explain more why please? We actually want to move in the other direction where the existence of these caches is relied on less, rather than more, and any caching happens outside. For example the Visual F# tools FSharp.Editor keeps its own table of options for documents/projects.

So why not have a table on the outside? Rather than relying on the keys of the incremental builders lookup?

At the moment, FSharpChecker.InvalidateConfiguration does not invalidate the typecheck cache (checkFileInProjectCache), which can lead to the use of outdated typecheck results for script files.

OK, interesting - perhaps these should be separate fixes please?

@auduchinok
Copy link
Member

So why not have a table on the outside? Rather than relying on the keys of the incremental builders lookup?

Thanks, it's our mid-term plan indeed, and I'm working on other things to make it feasible on our side. For the short-term, however, having such an API would the best way to fix several issues we're having by improving the invalidation for the scripts in projects. Getting cached entries from FCS is preferred, since we only want to invalidate what's actually in the builder cache and we should not request new script options for each script, since invalidations are expected to be fast and it may be very expensive to resolve referenced assemblies and packages.

@DedSec256 DedSec256 force-pushed the ber.a/ivalidateConfig branch from 8b57dbc to d945d9c Compare March 18, 2022 13:07
@DedSec256 DedSec256 changed the title Add GetCachedProjectOptions and invalidate typecheck cache when invalidating a configuration Add GetCachedProjectOptions Mar 18, 2022
@dsyme
Copy link
Contributor

dsyme commented Mar 19, 2022

Thanks, it's our mid-term plan indeed, and I'm working on other things to make it feasible on our side. For the short-term...

Given that anything we add to FCS has to stay, I think I'd prefer you move to your mid-term plan rather than adding to the FCS API? Or if really necessary use private reflection or some other hack?

So I'd be inclined to say we don't take this... thanks

@DedSec256 DedSec256 closed this Mar 22, 2022
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.

3 participants