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

Move importance/verbosity check earlier in logging #5992

Closed
Tracked by #5873
Forgind opened this issue Dec 29, 2020 · 2 comments · Fixed by #6381
Closed
Tracked by #5873

Move importance/verbosity check earlier in logging #5992

Forgind opened this issue Dec 29, 2020 · 2 comments · Fixed by #6381
Assignees
Labels
needs-design Requires discussion with the dev team before attempting a fix. performance size:3 triaged
Milestone

Comments

@Forgind
Copy link
Member

Forgind commented Dec 29, 2020

Currently, it seems the importance check waits until it reaches the BuildEngine. Might it make more sense to check that a lot earlier, say, in TaskLoggingHelper instead? It would be a little less centralized, and we'd have to expose it, but then we wouldn't have to create strings that we're never going to log, and we can perform some other little optimizations along the way if getting the information that we're not going to log would take a lot of time.

@Forgind Forgind added performance needs-triage Have yet to determine what bucket this goes in. labels Dec 29, 2020
@benvillalobos
Copy link
Member

Team Triage: We should have some performance profiling on this to justify the amount of work that would need to happen to get this to work.

@benvillalobos benvillalobos added needs-design Requires discussion with the dev team before attempting a fix. and removed needs-triage Have yet to determine what bucket this goes in. labels Jan 13, 2021
@rokonec
Copy link
Contributor

rokonec commented Apr 16, 2021

@ladipro This might be related to your current log level propagation optimization.

@ladipro ladipro self-assigned this Apr 16, 2021
@ladipro ladipro changed the title Move importance check earlier in logging? Move importance/verbosity check earlier in logging Apr 16, 2021
@ladipro ladipro added the size:3 label Apr 21, 2021
@ladipro ladipro moved this from To Do to In Progress in Perf sprint Apr 19th - May 2nd 2021 Apr 27, 2021
@ladipro ladipro moved this from In Progress to In Review in Perf sprint Apr 19th - May 2nd 2021 Apr 29, 2021
ladipro added a commit that referenced this issue Aug 5, 2021
Fixes #5992 

### Context

Some tasks spend a lot of time producing detailed log messages that end up going nowhere when building on the command line with Normal or Minimum verbosity. The goal of this PR is to expose an API for tasks to call to answer the question _"Are log messages of a given importance going to be consumed by a logger or should I not bother doing the work to create the output and skip calling `LogMessage` altogether?"_

### Changes Made

Added a public method named `LogsMessagesOfImportance` on `TaskLoggingHelper` to answer the question above. This PR focuses on command line scenarios such as `dotnet build` but the logic could be easily used/extended to work with 3rd party loggers (in IDEs for example) if we introduce a mechanism by which loggers promise to ignore too verbose messages. The solution presented herein is a bolt-on, making minimum changes to the existing logging infra. Third party loggers are assumed to be potentially logging everything so if at least one such logger is registered, the optimization is disabled.

The new method is used internally by `TaskLoggingHelper` and by the otherwise spammy RAR task.

### Testing

Existing and new unit tests, manual verification on command line as well as in Visual Studio.

Performance testing has shown ~18 ms improvement in RAR run-time alone when building a single project with `dotnet build`.

### Notes

This PR also introduces a new class `EngineServices` to serve as a replacement for the `IBuildEngineN` interfaces.
@ladipro ladipro moved this from In Review to Done in Perf sprint Apr 19th - May 2nd 2021 Dec 8, 2021
@ladipro ladipro added this to the VS 17.0 milestone Dec 9, 2021
@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-design Requires discussion with the dev team before attempting a fix. performance size:3 triaged
Development

Successfully merging a pull request may close this issue.

5 participants