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

Create explicit logger forwarder for buildcheck #10084

Merged
merged 1 commit into from May 2, 2024

Conversation

JanKrivanek
Copy link
Member

Contributes to #10068

Context

#9988 (comment) uncovered a perf regression with BuildCheck.
Portion of the regression was tracked to excessivbe logging of tasks (credit to @ladipro for the initial idea). This PR is trying to address the case.

Changes Made

BuildCheck is bringing in it's own forwarding logger - so that it can properly self-declare the minimum needed tasks verbosity (and in the future influence what events need to be transfered)

Testing

The verbosity integrations test
Pre-existing tests on BuildCheck

@JanKrivanek JanKrivanek requested a review from AR-May April 29, 2024 11:41
Copy link
Member

@AR-May AR-May left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@YuliiaKovalova YuliiaKovalova 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!

Is it possible to add some documentation to illustrate the logging process in MSBuild?

@JanKrivanek
Copy link
Member Author

Looks good!

Is it possible to add some documentation to illustrate the logging process in MSBuild?

Yeah - it's in my bucket list - but first I need to find the time to debug through the code back and forth - as there seems to be a bit more to it than publicly documented. For now the public documentation is what should siffice for basic knowledge: https://learn.microsoft.com/en-us/visualstudio/msbuild/writing-multi-processor-aware-loggers

@JanKrivanek JanKrivanek merged commit ef8b6a4 into dotnet:main May 2, 2024
10 checks passed
@JanKrivanek JanKrivanek deleted the proto/buildcheck-logger-fwder branch May 2, 2024 11:26
@AR-May
Copy link
Member

AR-May commented May 2, 2024

Measured perf of this fix. Results:

OC rebuild with analyzers with analyzers & fix without analyzers
median 85053 76666 73409
mean 88154 77012 73168

Despite this good numbers, the CPU of the main msbuild node still has a very considerable overhead when analyzers are on:
50762 MSec vs 33109 MSec. The out of proc nodes in average do not have any overhead.

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

Successfully merging this pull request may close these issues.

None yet

3 participants