Skip to content
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

Throttle async event queues #7784

Merged
merged 4 commits into from
Jan 6, 2016
Merged

Throttle async event queues #7784

merged 4 commits into from
Jan 6, 2016

Conversation

heejaechang
Copy link
Contributor

we had several reports saying we sometime OOM. investigation shows that we have a lot of async events pending especially around removing documents (ex, close solution, vs shutdown, solution re-analysis and etc).

pending events itself is not that much problem, but data captured for the events sometimes accumulated too much which can cause us to OOM.

this change puts a throttle to async events queue so that we block event producers if too many events are flooded to the queue. queue will be unblocked as pending events are processed by consumers.

...

I also changed some parts of engine not to raise unnecessary events.

@heejaechang
Copy link
Contributor Author

related issue - icsharpcode/RefactoringEssentials#159

@heejaechang
Copy link
Contributor Author

@srivatsn @basoundr @natidea @mavasani can you take a look?

@heejaechang
Copy link
Contributor Author

if this is not sufficient, we can make it a bit better by holding onto only last event and cancel out middle ones for same key (document, analyzer)

@heejaechang
Copy link
Contributor Author

@dotnet-bot retest prtest/mac/dbg/unit32 please

@heejaechang
Copy link
Contributor Author

retest mac test since the failure was timeout on cloning repo.

@@ -181,7 +181,7 @@ internal CompilationWithAnalyzers GetCompilationWithAnalyzers(Project project, C
var data = await _executor.GetSyntaxAnalysisDataAsync(userDiagnosticDriver, stateSet, versions).ConfigureAwait(false);
if (data.FromCache)
{
RaiseDiagnosticsCreated(StateType.Syntax, document.Id, stateSet, new SolutionArgument(document), data.Items);
RaiseDiagnosticCreatedFromCacheIfNeeded(StateType.Syntax, document, stateSet, data.Items);
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming: RaiseDiagnostics instead of RaiseDiagnostic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@srivatsn
Copy link
Contributor

srivatsn commented Jan 5, 2016

👍

@amcasey
Copy link
Member

amcasey commented Jan 5, 2016

Failure seems related:

Microsoft.VisualStudio.LanguageServices.UnitTests.Diagnostics.DiagnosticTableDataSourceTests.TestAggregatedDiagnostic (from Roslyn.VisualStudio.Services.UnitTests) 

Expected: Proj1, Proj2
Actual: Proj1

@heejaechang
Copy link
Contributor Author

it looks like race was always there, but surfaced due to this change where events are now throttled. currently testing fix for the race (basically explicitly check that events are fired)

the race itself already existed but this PR made the race to surface.
@heejaechang
Copy link
Contributor Author

@dotnet-bot retest prtest/win/dbg/eta please

@heejaechang
Copy link
Contributor Author

made eta to be retested. issues was immutable dll being locked by other process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants