Skip to content

Avoid delegate allocations for ImmutableArray<T>.Any#61729

Merged
sharwell merged 1 commit intodotnet:mainfrom
sharwell:rm-delegate-allocs-compiler
Jun 7, 2022
Merged

Avoid delegate allocations for ImmutableArray<T>.Any#61729
sharwell merged 1 commit intodotnet:mainfrom
sharwell:rm-delegate-allocs-compiler

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Jun 6, 2022

This is the compiler code portion of #61719.

@sharwell sharwell requested a review from a team as a code owner June 6, 2022 19:56
@ghost ghost added the Area-Compilers label Jun 6, 2022

// If no switch sections are subsumed, just return
if (!switchSections.Any(s => s.SwitchLabels.Any(l => isSubsumed(l))))
if (!switchSections.Any(static (s, reachableLabels) => s.SwitchLabels.Any(static (l, reachableLabels) => isSubsumed(l, reachableLabels), reachableLabels), reachableLabels))
Copy link
Copy Markdown
Member

@jcouv jcouv Jun 7, 2022

Choose a reason for hiding this comment

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

This has become hard to follow (lots of nested parens). I'd be tempted to leave the original. Or we could use local functions to factor the logic out.

Copy link
Copy Markdown
Contributor Author

@sharwell sharwell Jun 7, 2022

Choose a reason for hiding this comment

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

I kept thinking a language feature supporting this case would be especially helpful. For cases where static local functions were already used, I left those unchanged. However, these cases were noticeably more difficult to audit, so I did not proactively convert code from inline lambdas to local functions.

Only a very small number of cases had a significant readability impact, so I tend to prefer leaving them for now and perhaps they can be refactored in the future. It would be nice to have analyzer, IDE, and/or language support to help with the scenario.

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.

I'll submit a follow-up PR to simplify the inner call on this line. I'm not sure why our analyzer is failing to flag the lambda as unnecessarily complex, considering isSubsumed is already a static local function.

Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 1). Only one minor concern.

@jcouv jcouv self-assigned this Jun 7, 2022
Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 1)

@sharwell sharwell merged commit 5ada1d5 into dotnet:main Jun 7, 2022
@sharwell sharwell deleted the rm-delegate-allocs-compiler branch June 7, 2022 15:11
@ghost ghost added this to the Next milestone Jun 7, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants