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

Array enumerators are not preinitialized anymore #82993

Closed
MichalStrehovsky opened this issue Mar 5, 2023 · 1 comment · Fixed by #88371
Closed

Array enumerators are not preinitialized anymore #82993

MichalStrehovsky opened this issue Mar 5, 2023 · 1 comment · Fixed by #88371
Labels
area-NativeAOT-coreclr
Milestone
Future

Comments

@MichalStrehovsky
Copy link
Member

Noticed this by accident:
Hello world before #82751: ILC: Preinitialized 183 types out of 266.
After: ILC: Preinitialized 105 types out of 266.

  ILC: Could not preinitialize '[S.P.CoreLib]System.SZGenericArrayEnumerator`1<System.Collections.Generic.LowLevelDicti
  onary`2+Entry<System.RuntimeTypeHandle,Internal.TypeSystem.TypeDesc>>': Method '[S.P.CoreLib]System.SZGenericArrayEnu
  meratorBase..ctor(Array)', opcode 'stfld' Reference field

Previously, we were storing endIndex separately and passed a null array to the empty enumerator. Now we store an actual object reference and that aborts the optimization. Maybe we could lift that in the interpreter but that might open a can of worms.
@MichalStrehovsky MichalStrehovsky added this to the 8.0.0 milestone Mar 5, 2023
@ghost
Copy link

ghost commented Mar 5, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Noticed this by accident:
Hello world before #82751: ILC: Preinitialized 183 types out of 266.
After: ILC: Preinitialized 105 types out of 266.

  ILC: Could not preinitialize '[S.P.CoreLib]System.SZGenericArrayEnumerator`1<System.Collections.Generic.LowLevelDicti
  onary`2+Entry<System.RuntimeTypeHandle,Internal.TypeSystem.TypeDesc>>': Method '[S.P.CoreLib]System.SZGenericArrayEnu
  meratorBase..ctor(Array)', opcode 'stfld' Reference field

Previously, we were storing endIndex separately and passed a null array to the empty enumerator. Now we store an actual object reference and that aborts the optimization. Maybe we could lift that in the interpreter but that might open a can of worms.

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr
Milestone: 8.0.0

@agocke agocke added this to AppModel Mar 6, 2023
@jkotas jkotas mentioned this issue Mar 7, 2023
Open
@MichalStrehovsky MichalStrehovsky modified the milestones: 8.0.0, Future Apr 11, 2023
@MichalStrehovsky MichalStrehovsky mentioned this issue Apr 11, 2023
Closed
3 tasks
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 7, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 6, 2023
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this issue Jul 4, 2023
@MichalStrehovsky
Bring back old array enumerator code
0d316ae 
This is a 0.3% size saving for Stage1. We not only allow array enumerators to be preinitialized again, but also avoid introducing many array `MethodTables` (looks like the new enumerators force array MethodTables for cases where we could have avoided them).

I shaped the old code to look like after the refactor in dotnet#82751, but reintroduced the old classes.

Fixes dotnet#82993.
@MichalStrehovsky MichalStrehovsky mentioned this issue Jul 4, 2023
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 4, 2023
MichalStrehovsky added a commit that referenced this issue Jul 7, 2023
@MichalStrehovsky
Bring back old array enumerator code (#88371)
4285e43 
This is a 0.3% size saving for Stage1. We not only allow array enumerators to be preinitialized again, but also avoid introducing many array `MethodTables` (looks like the new enumerators force array MethodTables for cases where we could have avoided them).

Fixes #82993.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 7, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area-NativeAOT-coreclr
Projects
AppModel
Archived in project
Milestone
Future
1 participant
@MichalStrehovsky