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

Move type check of MD array to managed #103414

Merged
merged 4 commits into from
Jun 16, 2024
Merged

Conversation

huoyaoyuan
Copy link
Member

Removes a HELPER_METHOD_FRAME.

Generally there are more of MD array can be moved to managed, but not completely now.

Method Job Toolchain Mean Error StdDev Ratio RatioSD
SetCovariant Job-KZUIWY \PR\corerun.exe 2.758 ns 0.0252 ns 0.0236 ns 0.99 0.01
SetCovariant Job-ZSCMRL \main\corerun.exe 2.780 ns 0.0191 ns 0.0169 ns 1.00 0.00
SetExactMatch Job-KZUIWY \PR\corerun.exe 2.768 ns 0.0323 ns 0.0302 ns 0.99 0.02
SetExactMatch Job-ZSCMRL \main\corerun.exe 2.795 ns 0.0246 ns 0.0230 ns 1.00 0.00

Not over optimizing, just ensuring no regression.

@huoyaoyuan huoyaoyuan marked this pull request as ready for review June 13, 2024 12:12
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 13, 2024
Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@@ -1169,6 +1165,9 @@ DEFINE_METHOD(CASTHELPERS, CHKCASTCLASSSPECIAL, ChkCastClassSpecial, SM_Ptr
DEFINE_METHOD(CASTHELPERS, UNBOX, Unbox, SM_PtrVoid_Obj_RetRefByte)
DEFINE_METHOD(CASTHELPERS, STELEMREF, StelemRef, SM_ArrObject_IntPtr_Obj_RetVoid)
DEFINE_METHOD(CASTHELPERS, LDELEMAREF, LdelemaRef, SM_ArrObject_IntPtr_PtrVoid_RetRefObj)
#ifdef FEATURE_ARRAYSTUB_AS_IL
Copy link
Member Author

Choose a reason for hiding this comment

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

It seems that we are ready to enable it for all platforms. Just separating to another PR.

Copy link
Member

Choose a reason for hiding this comment

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

I agree it would be nice to enable this on all platforms and delete the #define.

@huoyaoyuan huoyaoyuan closed this Jun 16, 2024
@huoyaoyuan huoyaoyuan reopened this Jun 16, 2024
@jkotas
Copy link
Member

jkotas commented Jun 16, 2024

Not over optimizing, just ensuring no regression.

Could you please share the source for the benchmark? It is a bit suspicious that the difference between the baseline and the PR is in the noise range. I would expect it to be a small measurable improvement.

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.

LGTM otherwise. Thank you!

@huoyaoyuan
Copy link
Member Author

Updated test:

        private Base[,] baseArray = new Base[10, 1];
        private Base[,] covariantArray = new Derived[10, 1];
        private Derived[,] derivedArray = new Derived[10, 1];

        private Derived derivedObj = new Derived();

        private void GenericSet<T>(T[,] array, T value)
        {
            for (int i = 0; i < 10; i++)
            {
                array[i, 0] = value;
            }
        }

        [Benchmark]
        public void ReferenceTypeBase() => GenericSet(baseArray, derivedObj);

        [Benchmark]
        public void ReferenceTypeExactMatch() => GenericSet(derivedArray, derivedObj);

        [Benchmark]
        public void ReferenceTypeCovariant() => GenericSet(covariantArray, derivedObj);

Result:

Method Job Toolchain Mean Error StdDev Ratio RatioSD
ReferenceTypeBase Job-IECQRX \PR\corerun.exe 32.91 ns 0.183 ns 0.171 ns 0.76 0.01
ReferenceTypeBase Job-OMBWNE \main\corerun.exe 43.08 ns 0.219 ns 0.194 ns 1.00 0.00
ReferenceTypeExactMatch Job-IECQRX \PR\corerun.exe 29.27 ns 0.422 ns 0.395 ns 1.00 0.02
ReferenceTypeExactMatch Job-OMBWNE \main\corerun.exe 29.13 ns 0.342 ns 0.320 ns 1.00 0.00
ReferenceTypeCovariant Job-IECQRX \PR\corerun.exe 28.59 ns 0.229 ns 0.214 ns 1.00 0.01
ReferenceTypeCovariant Job-OMBWNE \main\corerun.exe 28.68 ns 0.181 ns 0.169 ns 1.00 0.00

Yes it is measuable improvement when array.GetType().ElementType isn't exact obj.GetType(). The static type of array doesn't matter here.

@jkotas jkotas merged commit eb455ec into dotnet:main Jun 16, 2024
87 of 89 checks passed
@huoyaoyuan huoyaoyuan deleted the array-method-stub branch June 16, 2024 07:18
@github-actions github-actions bot locked and limited conversation to collaborators Jul 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-VM-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