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.

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

[NativeAOT] Move IsByRefLikeFlag from RareFlags to ExtendedFlags #106155

Closed
wants to merge 1 commit into from

Conversation

filipnavara
Copy link
Member

The flag is only valid for value types, we have ample space in EETypeFlagsEx. This may avoid building the optional fields node in ILC and it's basically free tiny space optimization.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 8, 2024
Copy link
Contributor

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

@MichalStrehovsky
Copy link
Member

Heh, I was thinking about this too. One could potentially even overload the meaning of one of the existing bits (if they're not interpreted by someone else, like the GC), because some of the FlagsEx only have a meaning for a reference type. This one only has meaning for a value type. And it's not perf critical to access.

We might even be able to fit all optional fields into FlagsEx. The remaining RareFlags after this are all related to dynamic types, so I'd just move them to

and delete the RareFlags and OptionalFields concept completely.

Hopefully we won't need the "prime real estate" flags later again.

@filipnavara
Copy link
Member Author

One could potentially even overload the meaning of one of the existing bits

Ah, good observation!

We might even be able to fit all optional fields into FlagsEx.

I didn't go that far in exploring this, but presumably there's enough space to do that. There are two remaining things in the optional fields on EETypeNode:

  • Nullable value offset (only valid for IsNullable)
  • Value type field padding (only valid for value type)

Both should easily fit in the leftover bits unless they have some special GC meaning.

Not sure if I will go on and prototype this. There are some more pressing issues on my list. This one flag just seemed trivial enough to try.

@filipnavara
Copy link
Member Author

The failed test are relevant. I didn't notice that GenericDefinitionEETypeNode sets HasComponentSizeFlag and uses the lower bits.

@filipnavara filipnavara closed this Aug 8, 2024
@MichalStrehovsky
Copy link
Member

The failed test are relevant. I didn't notice that GenericDefinitionEETypeNode sets HasComponentSizeFlag and uses the lower bits.

It would probably be fine to store the information GenericDefinitionEETypeNode puts there into BaseSize instead. These are never allocated so GC won't mind. Either field is a misuse in the same way.

@filipnavara
Copy link
Member Author

Thanks for confirmation. That's what I thought. It was just no longer as trivial change as I thought.

@MichalStrehovsky
Copy link
Member

Thanks for confirmation. That's what I thought. It was just no longer as trivial change as I thought.

Would you mind if I pick this up and try shuffling these?

@filipnavara
Copy link
Member Author

Would you mind if I pick this up and try shuffling these?

That would be great! Thanks!

@github-actions github-actions bot locked and limited conversation to collaborators Sep 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants