Lazy types in caching #1757
Lazy types in caching #1757
Conversation
This reverts commit ad53c29.
Remove readonly
Remove readonly
…etes presions analysis session that was canceled may still be running.
| var (modulesCount, totalMilliseconds) = ActivityTracker.EndTracking(); | ||
| totalMilliseconds = Math.Round(totalMilliseconds, 2); | ||
| _analyzer.RaiseAnalysisComplete(modulesCount, totalMilliseconds); | ||
| _log?.Log(TraceEventType.Verbose, $"Analysis complete: {modulesCount} modules in {totalMilliseconds} ms."); |
There was a problem hiding this comment.
One thing I'm seeing here is that due to the lazy loading, we're (in a way) under-reporting the number of modules we're working with. Hopefully that's not such a big deal if the work also disappears, but it may end up appearing as though we're not "improving" per-module because for the N modules we still have to load, we have to do the same work.
There was a problem hiding this comment.
Not entirely happy with RaiseAnalysisComplete being bool, but I can make it return if it actually raised the event. Somewhat ugly, but implementation uses data internal to the analyzer which i'd rather not make public.
There was a problem hiding this comment.
I'm not sure what you mean by returning a bool, I'm more talking about the fact that we'll log "123 modules analyzed", when maybe there are a bunch more that weren't "loaded" because they'll be fetched from disk.
You don't have to change anything for this comment, I was more thinking about the impact of lazyness on our reporting.
There was a problem hiding this comment.
I mean returning bool from RaiseAnalysisComplete and then only reporting when it is really done.
| if (id == null) { | ||
| var parent = moduleResolution.CurrentPathResolver.GetModuleParentFromModuleName(moduleName); | ||
| if (parent == null) { | ||
| id = moduleName; |
There was a problem hiding this comment.
Won't this string just be something like "foo.bar" without any versioning? If the version needs to come from a parent, I'd expect to have seen recursion or something. Maybe I'm just reading this wrong.
There was a problem hiding this comment.
This is an edge case just to handle GetModuleParentFromModuleName that can return null since hashing needs parent IImportChildrenSource.
|
Caching isn't perfect for all libraries, but the default is None and we want the rest of the fixes too. Approving. |
Fixes #1728
Fixes #1758
Fixes #1514
Fixes #1515
Fixes #1516
Fixes #1517
Fixes #1518
Fixes #1569
Fixes #1680
posixormmap.TODO (later):
Related