-
Notifications
You must be signed in to change notification settings - Fork 417
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
Update Roslyn to 4.4.0 1.22369.1 #2420
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small suggestion, otherwise LGTM
src/OmniSharp.Roslyn.CSharp/Services/Completion/CompletionListBuilder_Sync.cs
Show resolved
Hide resolved
…Builder_Sync.cs Co-authored-by: Fred Silberberg <fred@silberberg.xyz>
Not sure what is going on here, or whether there is some regression in Roslyn, but we have a bunch of |
Thanks for helping fix up the scripting test @filipw! Going to keep working on it. |
@JoeRobich selectively running tests, it appears that import completion is taking significant amounts of memory. By far the worst offender is |
@@ -55,7 +55,7 @@ internal static partial class CompletionListBuilder | |||
completionTasksAndProviderNamesBuilder.Add(((Task<CompletionChange>?)completionService.GetChangeAsync(document, completion), providerName)); | |||
} | |||
} | |||
var completionTasksAndProviderNames = completionTasksAndProviderNamesBuilder.MoveToImmutable(); | |||
var completionTasksAndProviderNames = completionTasksAndProviderNamesBuilder.ToImmutable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@filipw why did you revert this? Just to see if it would fix the OOM issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it failed many tests with System.InvalidOperationException : MoveToImmutable can only be performed when Count equals Capacity.
One thing is we inject |
The import completion tests are not using the shared fixture (my latest commit adds a new fixture to share hosts between them). |
So, I made the assumption that the stack trace that is failing is also the stack that is allocating. I choose to look at the TestHelper GetReferences and by caching the MetaData References memory stay much, much lower.
|
Now to work out why the global line formatting options aren't applying to Cake services..
|
😅👏 |
The changes are mostly around how Global Options in Roslyn is changing. We need to provide fallbacks which derive from OmniSharpOptions for LineFormatting options that are normally found in the .editorconfig.