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

Log properties initial value #4192

Closed
wants to merge 7 commits into from
Closed

Log properties initial value #4192

wants to merge 7 commits into from

Conversation

aelij
Copy link

@aelij aelij commented Feb 21, 2019

Fixes #2711

@aelij
Copy link
Author

aelij commented Feb 21, 2019

@KirillOsenkov Please review and see if this answers your needs

@cdmihai
Copy link
Contributor

cdmihai commented Feb 25, 2019

This change is on the hot perf path, it would be good to see perf numbers comparing the following scenarios with master msbuild:

  • building a big repo (e.g. orchard core) with various logger setups (no loggers, file logger on default verbosity, file logger on debug verbosity, binary logger)
  • dotnet CLI verbs on the basic project templates

@danmoseley
Copy link
Member

As well as more work on the hot path, it also means more logging on the hot path, even if a logger is not attached or would ignore the messages (due to verbosity). There are other places in the codebase where the engine "cheats", this is the pattern:

                    if (!_loggingService.OnlyLogCriticalEvents)
                    {
                        ... log informational things
                    }

I would wrap as above, and still measure after that as this is such a critical hot path where most of the time this work is not needed.

@aelij
Copy link
Author

aelij commented Mar 31, 2019

@danmosemsft I added the checks you suggested.

@danmoseley
Copy link
Member

@aelij thanks, it's not my repo, but I think they will need solid perf measurements as mentioned above. This path is perf critical not just to MSBuild but also Visual Studio project load, etc. This will have an impact as it's adding more code the question is how much.

@livarcocc
Copy link
Contributor

@benvillalobos Let's do a private build of this branch and kick a VS insertion to run RPS on to double check for perf regressions.

@AndyGerlicher I think this would be a good one for our teams to review together given your heavy usage of evaluation.

@cdmihai
Copy link
Contributor

cdmihai commented Jul 4, 2019

@maneely Maybe it would be good to merge this PR with yours (#4461), since they're doing very related work, and the mechanism would be the same.

@rainersigwald rainersigwald requested a review from maneely July 9, 2019 20:53
@rainersigwald
Copy link
Member

@maneely Matt Neely FTE Maybe it would be good to merge this PR with yours (#4461), since they're doing very related work, and the mechanism would be the same.

Team triage: let's do that. Please tag @aelij on the PR. Since that PR is behind a feature flag, it alleviates the performance concerns we have with this approach and doing it all the time.

Thanks @aelij.

@maneely
Copy link
Contributor

maneely commented Jul 16, 2019

Will do. Thx.

@aelij aelij deleted the logprops branch July 17, 2019 06:03
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.

Log how properties are initially assigned
6 participants