Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Reducing list allocations and copying#11838

Merged
stephentoub merged 1 commit intodotnet:masterfrom
bartdesmet:Issue-11837
Sep 30, 2016
Merged

Reducing list allocations and copying#11838
stephentoub merged 1 commit intodotnet:masterfrom
bartdesmet:Issue-11837

Conversation

@bartdesmet
Copy link
Copy Markdown
Contributor

This addresses issue #11837. In places where keeping track of the index of the next element to write into the array is rather clumsy, I'm using a small ArrayBuilder<T> helper type which exposes an Add method, so the original List<T>-like code keeps working and no index tracking is needed. This utility is also responsible to either return the array or return a TrueReadOnlyCollection<T> wrapping it (ownership transfer semantics), so we can pass it along to e.g. an expression factory method without causing it to create an unnecessary copy.

var index = (IndexExpression)_left;

var vars = new List<ParameterExpression>(index.ArgumentCount + 2);
var exprs = new List<Expression>(index.ArgumentCount + 3);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is exactly the case that turns out to be more expensive than it should be. We know the size upfront, yet use a List<T> so we don't have to track an index into the array to assign to next (which is non-trivial due to the branching below). We then went on to pass the List<T> to the Block factory as-is, causing a copy to be made in its call to ToReadOnly. Given these collections exhibit ownership transfer behavior, we should be able to just create a TrueReadOnlyCollection<T> wrapper around a T[] and avoid any copies.

if (NearestHoistedLocals != null && vars.Count > 0)
{
// Find what array each variable is on & its index
var indexes = new List<long>(vars.Count);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This turned out to be an unnecessary allocation when there are no variables. We created the list anyway and then went on to check the length of the list which gets populated in a loop with fixed iteration count and no conditionals inside, only to conclude it was throw-away work. Restructuring the conditionals here.

@bartdesmet
Copy link
Copy Markdown
Contributor Author

Test Innerloop Ubuntu14.04 Release Build and Test please (https://github.com/dotnet/corefx/issues/11851)

@karelz karelz added enhancement Product code improvement that does NOT require public API changes/additions area-System.Linq.Expressions labels Sep 26, 2016
@stephentoub
Copy link
Copy Markdown
Member

LGTM. Can you rebase to address the merge conflict?

@bartdesmet
Copy link
Copy Markdown
Contributor Author

This is good to go now as well.

@bartdesmet
Copy link
Copy Markdown
Contributor Author

Test Innerloop CentOS7.1 Release Build and Test please (failed to mkdirs)

@stephentoub stephentoub merged commit 8378243 into dotnet:master Sep 30, 2016
@bartdesmet bartdesmet deleted the Issue-11837 branch September 30, 2016 18:55
@karelz karelz modified the milestone: 1.2.0 Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Reducing list allocations and copying

Commit migrated from dotnet/corefx@8378243
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Linq.Expressions enhancement Product code improvement that does NOT require public API changes/additions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants