Skip to content
This repository was archived by the owner on Apr 14, 2022. It is now read-only.

Remove all references to dropped modules and force GC on reload #1402

Merged
jakebailey merged 14 commits into from
Aug 5, 2019
Merged

Remove all references to dropped modules and force GC on reload #1402

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
521eb47
First attempt at ensuring moduled dropped during reload are GC'd
jakebailey Jul 30, 2019
74dec25
Make sure AST is set after content is cleared
jakebailey Jul 31, 2019
6abeb90
More tests passing
jakebailey Jul 31, 2019
ddfb305
ModuleWalker updates
jakebailey Jul 31, 2019
bfc4955
Also don't merge against specialized modules, do builtin _astMap rese…
jakebailey Aug 1, 2019
2425d1e
Don't call dispose directly, it's done elsewhere and has no effect
jakebailey Aug 1, 2019
e843a60
Force GC via normal session instead of waiting for a hardcoded time
jakebailey Aug 2, 2019
30bd001
Un-refactor function since it's not being called externally anymore
jakebailey Aug 2, 2019
0336dfb
Formatting/usings
jakebailey Aug 2, 2019
4c51603
PR feedback
jakebailey Aug 2, 2019
fbed27b
Undo IsBuiltin, some things continued to be referenced with that changed
jakebailey Aug 5, 2019
540676f
Move reload logic down into PythonAnalyzer, remove added public infer…
jakebailey Aug 5, 2019
490991b
Mode AddReference to PythonType as override
jakebailey Aug 5, 2019
ac2bb01
Remove dead code, make all ResetAnalyzer calls full
jakebailey Aug 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/Analysis/Ast/Impl/Analyzer/Definitions/IPythonAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,9 @@ public interface IPythonAnalyzer {


/// <summary>
/// Removes all the modules from the analysis and restarts it.
/// Removes all the modules from the analysis and restarts it, including stubs.
/// </summary>
/// <param name="full">True if everything should be dropped, including closed files and stubs.</param>
Task ResetAnalyzer(bool full);
Task ResetAnalyzer();

/// <summary>
/// Returns list of currently loaded modules.
Expand Down
18 changes: 8 additions & 10 deletions src/Analysis/Ast/Impl/Analyzer/PythonAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,18 +196,16 @@ public IReadOnlyList<DiagnosticsEntry> LintModule(IPythonModule module) {
return optionsProvider?.Options?.LintingEnabled == false ? Array.Empty<DiagnosticsEntry>() : result;
}

public async Task ResetAnalyzer(bool full) {
if (full) {
var interpreter = _services.GetService<IPythonInterpreter>();
var builtins = interpreter.ModuleResolution.BuiltinsModule;
builtins.SetAst(builtins.Analysis.Ast);

await interpreter.TypeshedResolution.ReloadAsync();
await interpreter.ModuleResolution.ReloadAsync();
}
public async Task ResetAnalyzer() {
var interpreter = _services.GetService<IPythonInterpreter>();
var builtins = interpreter.ModuleResolution.BuiltinsModule;
builtins.SetAst(builtins.Analysis.Ast);

await interpreter.TypeshedResolution.ReloadAsync();
await interpreter.ModuleResolution.ReloadAsync();

lock (_syncObj) {
_forceGCOnNextSession = _forceGCOnNextSession || full;
_forceGCOnNextSession = true;
Copy link
Contributor

@AlexanderSher AlexanderSher Aug 5, 2019

Choose a reason for hiding this comment

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

If we reload TypeshedResolution and BuiltinsModule, there is no reason for a split bleow. _analysisEntries should be completely cleared, _dependencyResolver can be thrown away.

Copy link
Member Author

@jakebailey jakebailey Aug 5, 2019

Choose a reason for hiding this comment

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

Sort of; the way we create the builtins module is really contrived and currently can only be created once when the interpreter object is created. What I had to do here is only a partial fix; we don't have a way to do a complete reload including the builtin module, so I kept the existing code and allowed it to remove more.

I'd like for this to be fixed in the future so we can have a "real" reload that drops everything but file contents and does a complete restart, but I think that'd be a bigger change.


_analysisEntries.Split(kvp => kvp.Value.Module is IBuiltinsPythonModule, out var entriesToPreserve, out var entriesToRemove);
_analysisEntries.Clear();
Expand Down
38 changes: 7 additions & 31 deletions src/LanguageServer/Impl/Implementation/Server.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,9 +178,9 @@ public void DidChangeConfiguration(DidChangeConfigurationParams @params, Cancell
_disposableBag.ThrowIfDisposed();
switch (@params.settings) {
case ServerSettings settings: {
if (HandleConfigurationChanges(settings)) {
RestartAnalysis();
}
Settings = settings;
_symbolHierarchyMaxSymbols = Settings.analysis.symbolsHierarchyMaxSymbols;
_completionSource.Options = Settings.completion;
break;
}
default:
Expand All @@ -199,27 +199,6 @@ private void DisplayStartupInfo() {
: Resources.InitializingForPythonInterpreter.FormatInvariant(_interpreter.Configuration.InterpreterPath));
}

private bool HandleConfigurationChanges(ServerSettings newSettings) {
var oldSettings = Settings;
Settings = newSettings;

_symbolHierarchyMaxSymbols = Settings.analysis.symbolsHierarchyMaxSymbols;
_completionSource.Options = Settings.completion;

if (oldSettings == null) {
return true;
}

if (!newSettings.analysis.errors.SetEquals(oldSettings.analysis.errors) ||
!newSettings.analysis.warnings.SetEquals(oldSettings.analysis.warnings) ||
!newSettings.analysis.information.SetEquals(oldSettings.analysis.information) ||
!newSettings.analysis.disabled.SetEquals(oldSettings.analysis.disabled)) {
return true;
}

return false;
}

private IDocumentationSource ChooseDocumentationSource(string[] kinds) {
if (kinds == null) {
return new PlainTextDocumentationSource();
Expand Down Expand Up @@ -261,24 +240,21 @@ private void ResetPathWatcher() {
if (_searchPaths == null || !_searchPaths.SequenceEqual(paths)) {
_searchPaths = paths;
_pathsWatcher?.Dispose();
_pathsWatcher = new PathsWatcher(_searchPaths, () => NotifyPackagesChanged(), _log);
_pathsWatcher = new PathsWatcher(_searchPaths, NotifyPackagesChanged, _log);
}
}

public void NotifyPackagesChanged(CancellationToken cancellationToken = default) {
private void NotifyPackagesChanged() {
_log?.Log(TraceEventType.Information, Resources.ReloadingModules);

_services.GetService<PythonAnalyzer>().ResetAnalyzer(true).ContinueWith(t => {
_services.GetService<PythonAnalyzer>().ResetAnalyzer().ContinueWith(t => {
if (_watchSearchPaths) {
ResetPathWatcher();
}

_log?.Log(TraceEventType.Information, Resources.Done);
_log?.Log(TraceEventType.Information, Resources.AnalysisRestarted);
}).DoNotWait();
}

private void RestartAnalysis() {
_services.GetService<PythonAnalyzer>().ResetAnalyzer(false).DoNotWait();
}
}
}