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.

Sign up for GitHub

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

Jump to bottom

BuildManager instances acquire its own BuildTelemetry instance #8444

Merged
merged 2 commits into from
Feb 17, 2023
Merged

BuildManager instances acquire its own BuildTelemetry instance #8444

merged 2 commits into from
Feb 17, 2023

Conversation

rokonec
Copy link
Contributor

@rokonec rokonec commented Feb 13, 2023

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1708215

Context

In VS there are multiple instances of BuildManager called asynchronously. For DTB and normal build and maybe other which I have not identified yet.

Changes Made

BuildManager instances acquire its own BuildTelemetry instance as oppose to sharing single BuildTelemetry instance in non thread safe manner.

Testing

Locally

ladipro
ladipro approved these changes Feb 14, 2023
View reviewed changes
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Looks great! So if I understand it correctly there is an implicit assumption that KnownTelemetry.BuildTelemetry is used serially / only on the main thread. And without your fix this was the only place violating the rule.

@rokonec
Copy link
Contributor Author

rokonec commented Feb 14, 2023

So if I understand it correctly there is an implicit assumption that KnownTelemetry.BuildTelemetry is used serially

Yes KnownTelemetry.BuildTelemetry is not thread safe and shall be used serially only to enrich telemetry data of soon to be started build. Current code does it. By creating new instance for each BuildManager, build manager code do handle it in thread safe manner.

BTW, in last commit I have renamed BuildTelemetry to PartialBuildTelemetry to better describe its intent.

JanKrivanek
JanKrivanek approved these changes Feb 15, 2023
View reviewed changes
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good!

src/Build/BackEnd/BuildManager/BuildManager.cs Show resolved Hide resolved
@rokonec rokonec added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Feb 16, 2023
@JaynieBai JaynieBai merged commit c5cceaa into dotnet:main Feb 17, 2023
rokonec added a commit to rokonec/msbuild that referenced this pull request Mar 10, 2023
@rokonec
BuildManager instances acquire its own BuildTelemetry instance (dotne…
1f83c7d 
…t#8444)

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1708215

Context
In VS there are multiple instances of BuildManager called asynchronously. For DTB and normal build and maybe other which I have not identified yet.

Changes Made
BuildManager instances acquire its own BuildTelemetry instance as oppose to sharing single BuildTelemetry instance in non thread safe manner.

Testing
Locally
@rokonec rokonec mentioned this pull request Mar 10, 2023
Merged
rokonec added a commit that referenced this pull request Mar 11, 2023
@rokonec
Merge pull request #8552 from rokonec/vs17.5
f6fdcf5 
Backported fix of concurrency Dictionary bug https://devdiv.visualstudio.com/DevDiv/_workitems/edit/795389?src=WorkItemMention&src-action=artifact_link. 
BuildManager instances acquire its own BuildTelemetry instance (#8444)
rokonec added a commit to rokonec/msbuild that referenced this pull request Mar 14, 2023
@rokonec
BuildManager instances acquire its own BuildTelemetry instance (dotne…
d89badb 
…t#8444)

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1708215

Context
In VS there are multiple instances of BuildManager called asynchronously. For DTB and normal build and maybe other which I have not identified yet.

Changes Made
BuildManager instances acquire its own BuildTelemetry instance as oppose to sharing single BuildTelemetry instance in non thread safe manner.

Testing
Locally
# Conflicts:
#	src/Build/BackEnd/Client/MSBuildClient.cs - resolved with minimal and safe approach
@rokonec rokonec mentioned this pull request Mar 14, 2023
Merged
rokonec added a commit that referenced this pull request Mar 14, 2023
@rokonec @AR-May
Concurrency bug fix - BuildManager instances acquire its own BuildTel…
f08e881 
…emetry instance (#8561)

* BuildManager instances acquire its own BuildTelemetry instance (#8444)

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1708215

Context
In VS there are multiple instances of BuildManager called asynchronously. For DTB and normal build and maybe other which I have not identified yet.

Changes Made
BuildManager instances acquire its own BuildTelemetry instance as oppose to sharing single BuildTelemetry instance in non thread safe manner.

Testing
Locally
# Conflicts:
#	src/Build/BackEnd/Client/MSBuildClient.cs - resolved with minimal and safe approach

* Bumping version

* Turn off static graph restore. (#8498)

Our CI builds fails because of bug NuGet/Home#12373. 
It is fixed in NuGet/NuGet.Client#5010. 
We are waiting for it to flow to CI machines. Meanwhile this PR applies a workaround.

Note: This PR needs to be reverted once it happens.

---------

Co-authored-by: AR-May <67507805+AR-May@users.noreply.github.com>
rokonec added a commit that referenced this pull request Mar 27, 2023
@dotnet-maestro-bot @rokonec @AR-May
[automated] Merge branch 'vs17.4' => 'vs17.5' (#8562)
8b68842 
* Concurrency bug fix - BuildManager instances acquire its own BuildTelemetry instance (#8561)

* BuildManager instances acquire its own BuildTelemetry instance (#8444)

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1708215

Context
In VS there are multiple instances of BuildManager called asynchronously. For DTB and normal build and maybe other which I have not identified yet.

Changes Made
BuildManager instances acquire its own BuildTelemetry instance as oppose to sharing single BuildTelemetry instance in non thread safe manner.

Testing
Locally
# Conflicts:
#	src/Build/BackEnd/Client/MSBuildClient.cs - resolved with minimal and safe approach

* Bumping version

* Turn off static graph restore. (#8498)

Our CI builds fails because of bug NuGet/Home#12373. 
It is fixed in NuGet/NuGet.Client#5010. 
We are waiting for it to flow to CI machines. Meanwhile this PR applies a workaround.

Note: This PR needs to be reverted once it happens.

---------

Co-authored-by: AR-May <67507805+AR-May@users.noreply.github.com>

* Use AutoResetEvent as oppose to ManualResetEventSlim (#8575)

Summary
Customer, mainly internal like XStore, with huge repos, using msbuild /graph /bl on powerful development and build computers, might experience 15x plus regression in evaluation time.

It has been identified as performance bug in our logging event pub/sub mechanism. When ingest queue reaches its bound, at .net472 ManualResetEventSlim causes way too many thread.Yields flooding the system with thread context switches.
This hypothesis has been verified by PerfMon perfcounter System.ContextSwitches.

Alhougt counterintuitive, AutoResetEvent , ManualResetEvent or even SpinLocking produced better behavior and with those the issue no longer reproduce.

Customer Impact
In case of XStore it was about 7 minutes in build time.

Regression?
Yes, introduced in VS 17.4.

Testing
Manual validation by @rokonec and automated tests. Using local repro to verify changes has fixed it.

Risk
Low

Note
It effect only VS MSBuild.exe. In dotnet build ManualResetEventSlim works better.

---------

Co-authored-by: Roman Konecny <rokonecn@microsoft.com>
Co-authored-by: AR-May <67507805+AR-May@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JanKrivanek JanKrivanek JanKrivanek approved these changes

@ladipro ladipro ladipro approved these changes

Assignees
No one assigned
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rokonec @JanKrivanek @ladipro @JaynieBai