Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Convert Array.IsPrimitiveTypeArray to C# - #27302

Closed
EgorBo wants to merge 2 commits into
dotnet:masterfrom
EgorBo:more-runtime-structs
Closed

EgorBo wants to merge 2 commits into
dotnet:masterfrom
EgorBo:more-runtime-structs

Conversation

@EgorBo

@EgorBo EgorBo commented Oct 19, 2019

Copy link
Copy Markdown
Member

Another baby steps towards https://github.com/dotnet/coreclr/issues/18111 (along with #27216)

Ported line by line C++ to C# for these structs (missing some debug asserts, #if FEATURE_PREJIT parts and I don't know to to set g_pEnumClass global in order to test if a type is an enum, also I am not sure in the pointer arithmetic e.g. when I do addr -2).

@jkotas I was just learning CoreCLR internals, let me know if it worth continuing or feel free to take over.

But it already works for me, e.g. I can obtain a correct CorElementType for any Array I tried.

@jkotas

jkotas commented Oct 19, 2019

Copy link
Copy Markdown
Member

Does this change make any measurable performance difference?

If we start duplicating the VM code in C# just because we can, it will tun into maintenance nightmare. For now, I would prefer to keep this duplication at minimum and do as needed only if there is a measurable benefit.

@jkotas

jkotas commented Oct 19, 2019

Copy link
Copy Markdown
Member

I don't know to to set g_pEnumClass global in order to test

I do not think we want to keep these in a global. Ideally, This should just be typeof(Enum).MethodTable or something like that. It is what we do in CoreRT.

@jkotas

jkotas commented Oct 19, 2019

Copy link
Copy Markdown
Member

#if FEATURE_PREJIT parts

Yep, these would need to be much more complex, otherwise this will break Samsung folks who stilll use fragile NGen.

@EgorBo

EgorBo commented Oct 19, 2019

Copy link
Copy Markdown
Member Author

@jkotas ok, makes sense, will wait for CI test results just to make sure it works
PS: Also, could re-use this code for Enum.InternalGetCorElementType internal call.
But yeah don't want to break anyone or increase maintenance costs.

@EgorBo EgorBo closed this Oct 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants