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

Implement Array.Initialize in C# #77336

Merged
merged 18 commits into from
Oct 25, 2022
Merged

Conversation

reflectronic
Copy link
Contributor

@reflectronic reflectronic commented Oct 22, 2022

Implements Array.Initialize for Mono, moves most of the implementation to managed code for CoreCLR, and adds a test.

For those using C++/CLI, your arrays shall now be initialized marginally faster:

Method Job Toolchain Mean Error StdDev Ratio RatioSD
Array_Initialize Job-LCNDMK \runtime-main 702.9 ns 5.08 ns 4.75 ns 5.79 0.05
Array_Initialize Job-BDKEGA \runtime 121.5 ns 0.79 ns 0.74 ns 1.00 0.00

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 22, 2022
@danmoseley
Copy link
Member

Are changes needed in Mono?

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

You will need to do something about the failing tests ... Array.Initialize is implemented on CoreCLR only currently.

@vargaz
Copy link
Contributor

vargaz commented Oct 23, 2022

The mono changes look ok.

@reflectronic
Copy link
Contributor Author

Test failures were caused by a System.Reflection.MetadataLoadContext test that scans all methods on an array type. Since a local variable of function pointer type was added to a method on System.Array, MLC would fail when decoding this local variable. Fortunately, the fix is pretty trivial, since we have a convenient place to move that to.

@@ -381,8 +381,58 @@ private unsafe bool IsValueOfElementType(object value)
// if this is an array of value classes and that value class has a default constructor
// then this calls this default constructor on every element in the value class array.
// otherwise this is a no-op. Generally this method is called automatically by the compiler
[MethodImpl(MethodImplOptions.InternalCall)]
public extern void Initialize();
public unsafe void Initialize()
Copy link
Member

@jkotas jkotas Oct 23, 2022

Choose a reason for hiding this comment

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

@dotnet/ilc-contrib Should Array.Initialize be marked with RequiresUnreferenceCode, or is the value type default constructor the special-cased by the trimmer and always preserved?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should:

var b = new ElementType[1];
b.Initialize();

struct ElementType
{
    public ElementType()
    {
        Console.WriteLine(".ctor called");
    }
}

dotnet run prints out ".ctor called".
But trimmed the app doesn't print out anything.

Related question - what should default do in this case, for example:

var c = new ElementType[1];
c[0] = default; // Should this call the default .ctor?

Running it seems like it will NOT call the .ctor - but what does it do?

Copy link
Member

Choose a reason for hiding this comment

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

Same goes for AOT - published as Native AOT the app above also doesn't print out anything.

Copy link
Contributor

@teo-tsirpanis teo-tsirpanis Oct 24, 2022

Choose a reason for hiding this comment

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

Related question - what should default do in this case, for example:

According to the specification, default ignores the parameterless constructor and generates a zeroed instance.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, adding RequiresUnreferencedCode on Array.Initialize has a ripple effect. It introduces warnings in situations where array type is passed into DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All).

I guess it will need more careful thought.

Copy link
Member

Choose a reason for hiding this comment

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

Opened #77426

@jkotas
Copy link
Member

jkotas commented Oct 23, 2022

LGTM modulo comments. Thank you! Another 100 lines of manually managed C++ bite the dust.

@@ -233,6 +233,31 @@ ves_icall_System_Array_SetValueRelaxedImpl (MonoArrayHandle arr, MonoObjectHandl
array_set_value_impl (arr, value, pos, FALSE, FALSE, error);
}

void
ves_icall_System_Array_InitializeInternal (MonoObjectHandleOnStack arr_handle, MonoError *error)
Copy link
Member

Choose a reason for hiding this comment

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

@reflectronic Are you up for implementing the mono version mostly in C#, too?

I think we can add something like NativeAOT's default constructor -> function pointer icall and then most of this code could be managed.

We don't need to do it in this PR (unless you want to). Just an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would have done it like that, but I couldn't quite find the function I wanted (wasn't sure if the thing I was looking at was safe to just call with calli, whether it would try to unbox the argument, etc.). Certainly, I could address it in a follow up.

Copy link
Member

Choose a reason for hiding this comment

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

@reflectronic I don't think there's an existing icall, but it should be ok to add something like this:

MonoMethod * const method = mono_class_get_method_from_name_checked (element_class, ".ctor", 0, 0, error);
if (!method) {
	return NULL;
}
gpointer addr = mono_compile_method_checked (method, error);
if (!is_ok (error)) {
	mono_error_cleanup (error);
	return NULL;
}
return mono_create_ftnptr(addr);

on the C# side it would look pretty much like the NativeAOT version - cast the icall's IntPtr result to a function pointer type and call it.

wasn't sure if the thing I was looking at was safe to just call with calli, whether it would try to unbox the argument, etc.

I'm pretty certain that it will just work. I believe it should be expecting an unboxed argument.

@build-analysis build-analysis bot mentioned this pull request Oct 25, 2022
2 tasks
@jkotas
Copy link
Member

jkotas commented Oct 25, 2022

The failure is known nuget infrastructure issue

@jkotas jkotas merged commit 54b12a8 into dotnet:main Oct 25, 2022
@reflectronic
Copy link
Contributor Author

Thanks for the reviews and feedback

@reflectronic reflectronic deleted the array-initialize branch October 29, 2022 20:22
@ghost ghost locked as resolved and limited conversation to collaborators Nov 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-TypeSystem-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.

9 participants