Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Performance] Reduce array allocations of BadArguments #69970

Merged
merged 6 commits into from
Sep 20, 2023
Merged

[Performance] Reduce array allocations of BadArguments #69970

merged 6 commits into from
Sep 20, 2023

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Sep 16, 2023 •
edited
Loading

Fixes #69969

This moves BadArguments from int array to a BitVector. This will eliminate allocations for most cases.

So, instead of having an array containing the indices of the bad arguments, we now create a BitVector whose true bits represent the bad arguments. This will eliminate the allocations pointed out in the linked issue.

The BitVector is not going to allocate for the vast majority of cases.

@Youssef1313 Youssef1313 requested a review from a team as a code owner September 16, 2023 08:20
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Sep 16, 2023
@ghost ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Sep 16, 2023
@Youssef1313
Copy link
Member Author

CI failure is unrelated:

D:\a_work\1\s\src\Compilers\VisualBasic\Portable\Microsoft.CodeAnalysis.VisualBasic.vbproj : error NU3037: Warning As Error: Package 'System.Numerics.Vectors 4.4.0' from source 'https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json': The repository primary signature validity period has expired. [D:\a_work\1\s\src\NuGet\Microsoft.Net.Compilers.Toolset\AnyCpu\Microsoft.Net.Compilers.Toolset.Package.csproj]
Failed to restore D:\a_work\1\s\src\Compilers\VisualBasic\Portable\Microsoft.CodeAnalysis.VisualBasic.vbproj (in 20.47 sec).

@CyrusNajmabadi
Copy link
Member

Can you show a before/after trace?

@jaredpar jaredpar removed the untriaged Issues and PRs which have not yet been triaged by a lead label Sep 18, 2023
@jaredpar jaredpar added this to the 17.9 milestone Sep 18, 2023
AlekseyTs
AlekseyTs reviewed Sep 19, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MemberAnalysisResult.cs Outdated
{
get
{
for (int i = 0; i < BadArguments.Capacity; i++)
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

for (int i = 0; i < BadArguments.Capacity; i++)

It looks like BitVector.TrueBits provides a more efficient way to find "true" bits. If the FirstBadArgument helper is meant to be used only in error scenarios, we might be fine with allocating the enumerator then. If enumerator's allocation is really a problem for error scenarios as well, we should consider moving this helper to BitVector and using TrueBits like way of looking for the answer. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

I switched to TrueBits().First()

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

AlekseyTs
AlekseyTs reviewed Sep 19, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs Outdated
@@ -3777,15 +3781,19 @@ internal static bool AreRefsCompatibleForMethodConversion(RefKind x, RefKind y,
// lambda binding in particular, for instance, with LINQ expressions.
// Note that BuildArgumentsForErrorRecovery will still bind some number
// of overloads for the semantic model.
Debug.Assert(badArguments == null);
Debug.Assert(badArguments == default);
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

== default

IsNull? #Closed

AlekseyTs
AlekseyTs reviewed Sep 19, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/MemberAnalysisResult.cs Outdated
/// For example, if a method overload has 5 parameters and the second parameter is the only bad parameter, then this
/// BitVector could end up with Capacity being 2 where BadArguments[0] is false and BadArguments[1] is true.
/// </remarks>
public readonly BitVector BadArguments;
Copy link
Contributor

@AlekseyTs AlekseyTs Sep 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

BadArguments

I am not sure why the member and the parameter got renamed. It looks like they might hold default value. #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought the "Opt" suffix is an old convention that is no longer used, so renamed it as I'm already changing these lines.

Should I revert the rename?

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the "Opt" suffix is an old convention that is no longer used

As far as I know, this isn't the case. The suffix is not required, but also is no forbidden.

Should I revert the rename?

I think this will be the right thing to do. It sounds like there a no good reasons for the rename.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 5)

AlekseyTs
AlekseyTs approved these changes Sep 20, 2023
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 6)

@AlekseyTs
Copy link
Contributor

@cston, @dotnet/roslyn-compiler For the second review

@333fred 333fred merged commit 57ebb12 into dotnet:main Sep 20, 2023
25 checks passed
@ghost ghost modified the milestones: 17.9, Next Sep 20, 2023
@333fred
Copy link
Member

333fred commented Sep 20, 2023

Thanks @Youssef1313!

@Youssef1313 Youssef1313 deleted the reduce-int-array-allocations branch September 21, 2023 00:20
@allisonchou allisonchou mentioned this pull request Sep 21, 2023
Closed
@Cosifne Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@333fred 333fred 333fred approved these changes

Assignees

@cston cston

Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Milestone
17.8 P3
Development

Successfully merging this pull request may close these issues.

Reduce int[] allocations from BadArguments
7 participants
@Youssef1313 @CyrusNajmabadi @AlekseyTs @333fred @jaredpar @cston @Cosifne