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

Add array initialization optimization #62392

Merged
merged 8 commits into from
Jul 15, 2022
Merged

Add array initialization optimization #62392

merged 8 commits into from
Jul 15, 2022

Conversation

adamperlin
Copy link
Contributor

Addresses #61011. In the case where a 1-dimensional array is created with an initializer, emit a call to System.GC.AllocateUninitializedArray instead of a newarr instruction to avoid performing an unnecessary zero-initialization.

@adamperlin adamperlin changed the title Add array initialization optimization fix and initial unit tests Add array initialization optimization Jul 6, 2022
333fred
333fred reviewed Jul 6, 2022
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

Overall this looks ready for review, just a few minor comments.

src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs Outdated Show resolved Hide resolved
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs Outdated Show resolved Hide resolved
src/Compilers/Core/Portable/WellKnownMembers.cs Outdated Show resolved Hide resolved
@adamperlin adamperlin marked this pull request as ready for review July 6, 2022 21:29
@adamperlin adamperlin requested a review from a team as a code owner July 6, 2022 21:29
@adamperlin adamperlin requested a review from 333fred July 6, 2022 21:39
333fred
333fred reviewed Jul 7, 2022
View reviewed changes
Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise this is looking good.

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTests.cs Outdated Show resolved Hide resolved
333fred
333fred approved these changes Jul 8, 2022
View reviewed changes
Copy link
Member

@333fred 333fred 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 7). @dotnet/roslyn-compiler for a second review.

Youssef1313
Youssef1313 reviewed Jul 8, 2022
View reviewed changes
Copy link
Member

@Youssef1313 Youssef1313 left a comment

Choose a reason for hiding this comment

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

Is there any test where AllocateUninitializedArray doesn't exist? This can be achieved by using MakeMemberMissing in a test.

@Youssef1313
Copy link
Member

@333fred Does VB need a similar change?

Youssef1313
Youssef1313 reviewed Jul 8, 2022
View reviewed changes
src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs
// Not always available.
continue;
}
if (wkm == WellKnownMember.Count) continue; // Not a real value.
Copy link
Member

Choose a reason for hiding this comment

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

@333fred Is it expected the test pass without skipping WellKnownMember.Count. That looks surprising to me.

Copy link
Member

Choose a reason for hiding this comment

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

See line 915: this continue is dead and will never be reached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this isn't related to my change at all I just noticed this statement was redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or yeah sorry unreachable I should say

@333fred
Copy link
Member

333fred commented Jul 8, 2022

Is there any test where AllocateUninitializedArray doesn't exist? This can be achieved by using MakeMemberMissing in a test.

Yes, you can do that via MakeMemberMissing, or it can be done by targetting a TFM that does not have the API (as is done in this PR).

@Youssef1313
Copy link
Member

Is there any test where AllocateUninitializedArray doesn't exist? This can be achieved by using MakeMemberMissing in a test.

Yes, you can do that via MakeMemberMissing, or it can be done by targetting a TFM that does not have the API (as is done in this PR).

Yeah I didn't see Issue61011_TFMDoesNotHaveMember earlier.

jcouv
jcouv reviewed Jul 12, 2022
View reviewed changes
Copy link
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 7). Only some nits to consider and a couple questions

@jcouv jcouv self-assigned this Jul 12, 2022
@adamperlin adamperlin requested a review from jcouv July 14, 2022 18:12
jcouv
jcouv approved these changes Jul 14, 2022
View reviewed changes
Copy link
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 8). Good to merge/squash

@adamperlin adamperlin merged commit 26a134e into main Jul 15, 2022
@ghost ghost added this to the Next milestone Jul 15, 2022
@adamperlin adamperlin deleted the adamperlin/bugfix-61011 branch July 15, 2022 18:45
333fred added a commit to 333fred/roslyn that referenced this pull request Jul 18, 2022
@333fred
Merge remote-tracking branch 'upstream/main' into forbid-inumber
cc0fc20 
* upstream/main: (40 commits)
  Revert "Generalize RoamingProfileStorageLocation to ClientSettingsStorageLocation (dotnet#62684)"
  Revert "Update SymReader to the latest version (dotnet#62695)"
  Apply changes directly to text buffer - take 2 (dotnet#62617)
  Allow diagnostic tagging to use either push or pull diagnostics
  Tests test and more tests
  Fix new rename showing overloads when not applicable (dotnet#62559)
  Add stronger type safety
  Add support for resolving contextual error types
  Pass LineFormatting options into OmniSharpCodeAction options
  Initial approach to supporting contextual types
  Update dependencies from https://github.com/dotnet/arcade build 20220715.4 (dotnet#62704)
  Handle diff3 conflict markers (dotnet#62391)
  Update SymReader to the latest version (dotnet#62695)
  Generalize RoamingProfileStorageLocation to ClientSettingsStorageLocation (dotnet#62684)
  Add IWorkspaceProjectContextFactory F# wappers (dotnet#62646)
  Use OmniSharp LineFormatting fallback options in more places.
  Remove dependency on Microsoft.VisualStudio.RemoteControl assembly from Features (dotnet#62655)
  Add array initialization optimization (dotnet#62392)
  [main] Update dependencies from dotnet/arcade (dotnet#62597)
  Keep selection on rename invocation (dotnet#62654)
  ...
adamperlin added a commit to adamperlin/roslyn that referenced this pull request Jul 19, 2022
@adamperlin
Add array initialization optimization (dotnet#62392)
4acaa46 
* Add array initialization optimization fix and initial unit tests

* fix: ensure indices are emitted for multidimensional arrays; add additional test cases and finish updating broken unit tests

* Address pre-review comments

* Remove extraneous whitespace

* Avoid PEVerify bug for test IL output by changing test .NET version depending on execution context

* Address final review comments and hopefully fix CI

* Final changes to get CI to pass

* Address additional review feedback
@MichalStrehovsky MichalStrehovsky mentioned this pull request Jul 22, 2022
Closed
333fred added a commit that referenced this pull request Jul 22, 2022
@333fred
Revert "Add array initialization optimization (#62392)"
333f942 
This reverts commit 26a134e.
@333fred 333fred mentioned this pull request Jul 22, 2022
Merged
333fred added a commit that referenced this pull request Jul 23, 2022
@333fred
Revert "Add array initialization optimization" (#62868)
625564f 
* Revert "Add array initialization optimization (#62392)"

This reverts commit 26a134e.

* Fix newly added test.
@adamsitnik adamsitnik mentioned this pull request Jul 25, 2022
Merged
@allisonchou allisonchou modified the milestones: Next, 17.4 P1 Jul 26, 2022
@ryzngard ryzngard mentioned this pull request Jul 27, 2022
Merged
@ltrzesniewski ltrzesniewski mentioned this pull request Aug 11, 2022
Closed
@jakobbotsch jakobbotsch mentioned this pull request Aug 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@333fred 333fred 333fred approved these changes

@Youssef1313 Youssef1313 Youssef1313 left review comments

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers
Projects
None yet
Milestone
17.4 P1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@adamperlin @Youssef1313 @333fred @jcouv @allisonchou