Investigate getting rid of locker object on LazyFormattedBuildEventArgs #6002
Assignees
Labels
Area: Logging
backlog
performance
Performance-Scenario-Build
This issue affects build performance.
Priority:2
Work that is important, but not critical for the release
size:1
triaged
Milestone
Comments
KirillOsenkov
added
performance
needs-triage
benvillalobos
removed
the
needs-triage
panopticoncentral
added
the
Priority:2
|
Labeling with size:1 as the cost of the initial investigation. Will open a follow-up issue if more work is identified. |
rainersigwald
pushed a commit
that referenced
this issue
Nov 23, 2021
Fixes #6002 ### Context As pointed out in #6002, the locker object is not necessarily needed in `LazyFormattedBuildEventArgs`. It guarantees that formatting the message and nulling out the `arguments` field happens atomically to prevent double-formatting. The rest of the code running under the lock is idempotent. ### Changes Made - Removed the locker object. - Simply removed locking from idempotent code. Compensated by making the `message` field volatile to guarantee thread safety. The `if (RawMessage == null) ... RawMessage =` pattern falls into this case. - Implemented formatting atomicity by replacing `object[] arguments` with `object argumentsOrFormattedMessage`. The lazy `Message` getter reads arguments from this field and writes the formatted message into the same field. The run-time type determines if the message is already formatted or not. ### Testing Existing unit tests, manual log and binlog smoke test. ### Notes - The locker object is part of public surface so technically this is a breaking change. - After `LazyFormattedBuildEventArgs.Message` runs the object may reference two strings - the original unformatted message and the newly created formatted one. This may have memory impact if the object is kept alive after the message is formatted.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We should strongly consider getting rid of https://source.dot.net/#Microsoft.Build.Framework/LazyFormattedBuildEventArgs.cs,e983fe598844d129,references
to reduce allocations. We allocate hundreds of thousands of these and we should just throw away the old guidance and lock on the event args object itself, or see if the lock is even necessary there (maybe there's a lock up the stack already).
The text was updated successfully, but these errors were encountered: