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

Add FirstEnabledTraitOrDefault helper method. #14444

Merged
merged 2 commits into from Dec 10, 2017

Conversation

Projects
None yet
3 participants
@RoosterDragon
Member

RoosterDragon commented Nov 27, 2017

This avoids the allocations caused by LINQ when using traits.FirstOrDefault(Exts.IsTraitEnabled). This is important in FrozenActorLayer.RefreshState which is called very often. We apply the new helper method to all areas using the old pattern. An overload that takes an array allows arrays to be enumerated without causing allocations.

Add FirstEnabledTraitOrDefault helper method.
This avoids the allocations caused by LINQ when using traits.FirstOrDefault(Exts.IsTraitEnabled). This is important in FrozenActorLayer.RefreshState which is called very often. We apply the new helper method to all areas using the old pattern. An overload that takes an array allows arrays to be enumerated without causing allocations.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Nov 28, 2017

Contributor

@RoosterDragon I suspect we have quite a few cases of traits.FirstOrDefault(t => !t.IsTraitDisabled) as well, would it win us anything to make those use this helper method, too?

Edit: Guess I was wrong, didn't see any cases like that which could have any meaningful impact.

Contributor

reaperrr commented Nov 28, 2017

@RoosterDragon I suspect we have quite a few cases of traits.FirstOrDefault(t => !t.IsTraitDisabled) as well, would it win us anything to make those use this helper method, too?

Edit: Guess I was wrong, didn't see any cases like that which could have any meaningful impact.

Fix bad uses of FirstEnabledTraitOrDefault on TraitInfos.
These are not traits so this method does not work. We can use EnabledByDefault on the ConditionalTraitInfo instead.
@RoosterDragon

This comment has been minimized.

Show comment
Hide comment
@RoosterDragon

RoosterDragon Dec 7, 2017

Member

Have fixed bad Exts.IsTraitEnabled calls affected by this PR. Remaining bad uses are covered in #14445.

Member

RoosterDragon commented Dec 7, 2017

Have fixed bad Exts.IsTraitEnabled calls affected by this PR. Remaining bad uses are covered in #14445.

public static T FirstEnabledTraitOrDefault<T>(this T[] ts)
{
// PERF: Avoid LINQ.
foreach (var t in ts)

This comment has been minimized.

@pchote

pchote Dec 9, 2017

Member

General question: Is the compiler smart enough to generate code to enumerate these by index (for (var i = 0; i < ts.Length; i++), or does it allocate and use an enumerator object? I'm assuming the former based on the PR description, but just want to double check.

@pchote

pchote Dec 9, 2017

Member

General question: Is the compiler smart enough to generate code to enumerate these by index (for (var i = 0; i < ts.Length; i++), or does it allocate and use an enumerator object? I'm assuming the former based on the PR description, but just want to double check.

This comment has been minimized.

@pchote

pchote Dec 10, 2017

Member

According to the internet: yes.

@pchote

pchote Dec 10, 2017

Member

According to the internet: yes.

This comment has been minimized.

@RoosterDragon

RoosterDragon Dec 11, 2017

Member

Yes, the compiler special cases foreach on arrays to do exactly that.

@RoosterDragon

RoosterDragon Dec 11, 2017

Member

Yes, the compiler special cases foreach on arrays to do exactly that.

@pchote

pchote approved these changes Dec 9, 2017

LGTM, but i'm interested in finding out the answer to my question above before hitting the merge button.

@pchote pchote added this to the Next release milestone Dec 9, 2017

@pchote pchote merged commit 5338784 into OpenRA:bleed Dec 10, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@RoosterDragon RoosterDragon deleted the RoosterDragon:first-enabled-trait-or-default branch Dec 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment