-
Notifications
You must be signed in to change notification settings - Fork 269
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
Lower reflection overhead #2839
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nohwnd
commented
May 10, 2024
nohwnd
commented
May 10, 2024
nohwnd
commented
May 10, 2024
nohwnd
commented
May 10, 2024
nohwnd
commented
May 10, 2024
nohwnd
commented
May 10, 2024
nohwnd
commented
May 10, 2024
nohwnd
commented
May 10, 2024
src/Adapter/MSTest.TestAdapter/Extensions/MethodInfoExtensions.cs
Outdated
Show resolved
Hide resolved
nohwnd
commented
May 10, 2024
nohwnd
commented
May 10, 2024
nohwnd
commented
May 28, 2024
test/UnitTests/MSTestAdapter.UnitTests/Discovery/TestMethodValidatorTests.cs
Show resolved
Hide resolved
Evangelink
reviewed
May 28, 2024
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.
I'll need another round of review and maybe playing a little with the code.
src/Adapter/MSTest.TestAdapter/Discovery/TestMethodValidator.cs
Outdated
Show resolved
Hide resolved
src/Adapter/MSTestAdapter.PlatformServices/Utilities/DeploymentItemUtility.cs
Show resolved
Hide resolved
Evangelink
approved these changes
May 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR simplifies the way we access reflection in discovery and run. By improving the attribute cache and using it whenever possible.
It also simplifies ReflectHelper code, by removing dependencies between methods, and giving them more descriptive names. To make it much easier to reason about the current and desired behavior.
I am keeping the existing non-consistent behavior, to avoid breaking changes.
IsAttributeDefined => IsNonDerivedAttributeDefined
GetCustomAttribute => GetFirstDerivedAttributeOrDefault + inherit: true - when used with attributes that allow only one instance
GetCustomAttributes -> GetDerivedAttributes + inherit: true - for attributes that allow multiple.
Before
Initial measurements, on SDK 3.3.1.
Discovery of 10k empty tests:
discovered: 10000 tests in 1992,1448 ms, sent them in 289,5684 ms, total: 2281,7132
discovered: 10000 tests in 899,1829 ms, sent them in 212,4562 ms, total: 1111,6391
discovered: 10000 tests in 552,0988 ms, sent them in 321,4334 ms, total: 873,5322
Run 10k empty tests:
discovery: 1327ms
Microsoft(R) Testing Platform Execution Command Line Tool
Version: 1.1.0+8c0a8fd8e (UTC 2024/04/03)
RuntimeInformation: win-x64 - .NET 8.0.5
Copyright(c) Microsoft Corporation. All rights reserved.
Passed! - Failed: 0, Passed: 10000, Skipped: 0, Total: 10000, Duration: 2s 272ms - mstest133.dll (win-x64 - .NET 8.0.5)
run: 2616ms
After:
After
Discover 10 empty tests:
discovered: 10000 tests in 1219,4222 ms, sent them in 227,5277 ms, total: 1446,9499 <<<
2088
discovered: 10000 tests in 324,3993 ms, sent them in 193,5436 ms, total: 517,9429 <<<
1024
discovered: 10000 tests in 259,1427 ms, sent them in 291,6613 ms, total: 550,804 <<<
discovered: discovery alloc: 50 MB send alloc: 72 MB
971
Run 10k empty tests:
discovery: 999ms
discovered: 10000 tests in 279,4147 ms, sent them in 333,1555 ms, total: 612,5702 <<<
discovered: discovery alloc: 49 MB send alloc: 72 MB
.NET Testing Platform v1.3.0-preview.24277.3+3e1e5aebb (UTC 2024/05/27) [win-x64 - .NET 8.0.5]
discovered: 10000 tests in 271,2012 ms, sent them in 186,4562 ms, total: 457,65739999999994 <<<
discovered: discovery alloc: 49 MB send alloc: 52 MB
Passed! - Failed: 0, Passed: 10000, Skipped: 0, Total: 10000, Duration: 1s 120ms - mstest133.dll (win-x64 - .NET 8.0.5)
run: 1475ms
Conclusion
The overhead is cut to about half of what it was before, both in memory allocations and speed.
Fix #2763