Replace CopyOnReadEnumerable with immutable collections #6176
Comments
KirillOsenkov
added
performance
needs-triage
|
I pulled on the string some more and ended up in ProjectInstance, where |
|
Russian Dolls: |
ladipro
added
backlog
performance
and removed
performance
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. |
Fixes #6176 Context The props and items sent to loggers as part of ProjectStartedEventArgs are passed in special collections holding snapshots of props/items that are created lazily when a logger starts enumerating them. These props/items are guaranteed to be deep copies of the real props/items such that no modifications that a logger may make don't propagate back. Changes Made For props, the collection holds tuples of prop name and evaluated value. Both are strings so the concept of deep copying is implicit (strings are immutable). Similarly, for items we return TaskItem instances created over the original item, which guarantees the deep-copy semantics (TaskItems holds a copy of the metadata and a few strings). Consequentially, there is no need to explicitly deep-copy the props/items before enumeration and the IDeepCloneable<T> interface becomes unused. As an additional code cleanup, ProjectPropertyInstanceEnumeratorProxy and ProjectItemInstanceEnumeratorProxy can easily be replaced with the Select LINQ operation and are thus not needed. Here's a detailed analysis of the code that runs when enumerating props and items, without and with these changes: Props Before: When a logger calls GetEnumerator CopyOnReadEnumerable creates a list and populates it with the results of calling DeepClone on the props. During enumeration the iterator in ProjectPropertyInstanceEnumeratorProxy extracts the Name and EvaluatedValue and returns them to the logger. After: When a logger calls GetEnumerator CopyOnReadEnumerable creates a list and populates it with [Name, EvaluatedValue] tuples, saving the intermediate step of cloning. Items Before: When a logger calls GetEnumerator CopyOnReadEnumerable creates a list and populates it with the results of calling DeepClone on the items. During enumeration the iterator in ProjectItemInstanceEnumeratorProxy creates new instances of TaskItem and returns them to the logger. After: When a logger calls GetEnumerator CopyOnReadEnumerable creates a list and populates it with [ItemType, TaskItem] tuples, saving the intermediate step of cloning. Semantically the old and new behaviors are equivalent. A snapshot of prop/items is made at the point where GetEnumerator is called. Testing Existing unit tests, some tests were modified.
|
@KirillOsenkov I have eliminated redundant deep cloning in the attached PR. I don't think it's worth optimizing further as this work/allocations are relatively minor. When building a medium sized solution with |
|
@ladipro nice, thanks! I've also found https://source.dot.net/#Microsoft.Build/BackEnd/Components/Logging/TargetLoggingContext.cs,d99a8dbf081f1a4d,references Should we do the same thing for target items too? Shall we file a separate issue? |
|
Also see related: Since items aren't truly immutable, we have a bug where we capture the live data structure for logging instead of capturing an immutable snapshot. If the core data structures are truly immutable, we can give an immutable snapshot to loggers without any copying whatsoever, this will also avoid allocations. |
|
Specifically, see this comment: msbuild/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs Lines 744 to 753 in a59d7a5 |
Yes to both! If you file an issue, may I ask you to include a high-level description of the MSBuild logging infra? How log events flow today and the shortcomings / inefficiencies you are aware of. |
|
OK here you go: #7142 |
Fixes dotnet#6176 Context The props and items sent to loggers as part of ProjectStartedEventArgs are passed in special collections holding snapshots of props/items that are created lazily when a logger starts enumerating them. These props/items are guaranteed to be deep copies of the real props/items such that no modifications that a logger may make don't propagate back. Changes Made For props, the collection holds tuples of prop name and evaluated value. Both are strings so the concept of deep copying is implicit (strings are immutable). Similarly, for items we return TaskItem instances created over the original item, which guarantees the deep-copy semantics (TaskItems holds a copy of the metadata and a few strings). Consequentially, there is no need to explicitly deep-copy the props/items before enumeration and the IDeepCloneable<T> interface becomes unused. As an additional code cleanup, ProjectPropertyInstanceEnumeratorProxy and ProjectItemInstanceEnumeratorProxy can easily be replaced with the Select LINQ operation and are thus not needed. Here's a detailed analysis of the code that runs when enumerating props and items, without and with these changes: Props Before: When a logger calls GetEnumerator CopyOnReadEnumerable creates a list and populates it with the results of calling DeepClone on the props. During enumeration the iterator in ProjectPropertyInstanceEnumeratorProxy extracts the Name and EvaluatedValue and returns them to the logger. After: When a logger calls GetEnumerator CopyOnReadEnumerable creates a list and populates it with [Name, EvaluatedValue] tuples, saving the intermediate step of cloning. Items Before: When a logger calls GetEnumerator CopyOnReadEnumerable creates a list and populates it with the results of calling DeepClone on the items. During enumeration the iterator in ProjectItemInstanceEnumeratorProxy creates new instances of TaskItem and returns them to the logger. After: When a logger calls GetEnumerator CopyOnReadEnumerable creates a list and populates it with [ItemType, TaskItem] tuples, saving the intermediate step of cloning. Semantically the old and new behaviors are equivalent. A snapshot of prop/items is made at the point where GetEnumerator is called. Testing Existing unit tests, some tests were modified.
The whole machinery around ProjectPropertyInstanceEnumeratorProxy, ProjectItemInstanceEnumeratorProxy, ItemDictionary and CopyOnReadEnumerable could really benefit from immutable collections.
A lot of unnecessary work and allocations are happening to enumerate properties and items.
The text was updated successfully, but these errors were encountered: