-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Keep native order of get members call from Type #69506
Conversation
Tagging subscribers to this area: @dotnet/area-system-reflection Issue DetailsKeep native order (declaration order) of members call in Type, for example
|
Author: | buyaa-n |
---|---|
Assignees: | - |
Labels: |
|
Milestone: | - |
src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System/Reflection/RuntimeTypeMemberCacheTests.cs
Outdated
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs
Outdated
Show resolved
Hide resolved
Could you please try a test case with a few hundred methods? I do not think that the VM method iterators that this using under the covers guarantee order. A test case with a few hundred methods should be large enough to prove or disprove whether it is the case. |
Sure, will add a test |
Yes, you are right, it is divided into chunks and returns methods starting from last chunk 😢 |
Seems we need to go back to the reordering approach, do not see any other/better way |
We do not need to. The runtime can iterate methods in native order, using the same low-level metadata iterators that are used for properties. We need to switch to use these order-preserving iterators for the Alternatively, we may be able to fix the runtime to keep the methods in chunks in native order and keep using the existing FCalls/QCalls. |
I was able to load methods in order using this approach, but there is other test failures with The
apparently methods are not getting populated completely with this approach, any suggestions @jkotas?
This option seems needs quite a lot change in native code, I would try it out if the above option would not work |
It looks like a problem with generic instantiations. I am not able to tell where the problem is by just looking at the code.
I do not think it would be a lot of code change. The change to start would be to change runtime/src/coreclr/vm/class.cpp Lines 2702 to 2704 in 4f01044
|
51c2a31
to
58a2cac
Compare
Thanks! that worked well 💯, I was thinking to add another pointer for opposite direction and a new iterator looping through them 😛 😆 |
src/libraries/System.Runtime/tests/System/Reflection/RuntimeTypeMemberCacheTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you!
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
I have triggered extra-plaforms to verify that the new tests are passing everywhere. |
All of them failed with Nuget error, requesting rerun not working, not sure what should do
CC @ViktorHofer |
@buyaa-n that was due to an AzDO change. I mail was sent out for it. The issue should be mitigated by now. |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
Still some tests failing with similar error, rerunning did not help, will try one more time |
/azp run runtime-extra-platforms |
Azure Pipelines successfully started running 1 pipeline(s). |
Is there an issue opened for these error? It is sufficient to note the know issues that are tracking the failures in the PR comment. You do not have to keep rerunning until you hit green. |
All unsuccessful builds on |
Keep native order (declaration order) of members call in Type, for example
GetProperties()
The issue causing this behavior is the member cache logic in the managed code, it caches members by first come first added order. The native call returns members in declared order as needed, but if an individual members called before all members call
the native (declared) order will be lost
After investigating the behavior and discussing the solution with the team we decided to keep the native call results order in the cache instead of adding an reordering logic by token or something else. The main goal is to not regress current performance. And that goal is achieved
The entire caching logic is in MergeWithGlobalList(T[] list) method and it is used only in
Insert(ref T[] list, string? name, MemberListType listType)
method.The method merges members loaded from native with cache depending on
MemberListType
. By my investigation the ordering is matter only forMemberListType.All
case, all other cases are loading only one member or a group of member with same name which would keep the order correctly.So I have created a
MergeWithGlobalListInOrder(T[] list)
method forMemberListType.All
case, the performance test result shows the new method has about the same perf as existing one forMemberListType.All
case, but not good for all other cases. But as I mentioned we don't need to change order in other cases.Fixes #46272