Skip to content

Add TestMetadataCache to improve CompositeWorkItem build speed - #4300

Merged
manfred-brands merged 1 commit into
nunit:masterfrom
lahma:optimize-test-level-attributes
Apr 13, 2023
Merged

Add TestMetadataCache to improve CompositeWorkItem build speed#4300
manfred-brands merged 1 commit into
nunit:masterfrom
lahma:optimize-test-level-attributes

Conversation

@lahma

@lahma lahma commented Feb 26, 2023

Copy link
Copy Markdown
Contributor

Addresses high memory usage reported in #3858 . Adds TestCache the same way there is MethodInfoCache and caches the action attributes just like in method info cache - other cached items could be added later if needed.

BenchmarkDotNet=v0.13.5, OS=Windows 11 (10.0.22621.1343/22H2/2022Update/SunValley2)
AMD Ryzen 9 5950X, 1 CPU, 32 logical and 16 physical cores
.NET SDK=6.0.114
  [Host]     : .NET 6.0.14 (6.0.1423.7309), X64 RyuJIT AVX2
  DefaultJob : .NET 6.0.14 (6.0.1423.7309), X64 RyuJIT AVX2

Before

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
TestActionUsage 161.4 ms 3.19 ms 5.76 ms 750.0000 500.0000 500.0000 386.22 MB

After

Method Mean Error StdDev Gen0 Gen1 Allocated
TestActionUsage 8.975 ms 0.1141 ms 0.1068 ms 281.2500 140.6250 4.71 MB

@mikkelbu mikkelbu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for providing this @lahma. I'm a bit tired today, so I cannot determine if we will run into some of the same problems as we did for the other cache - see e.g. #4104 for the solution.

I'll need to examine this some more

Comment thread src/NUnitFramework/framework/Internal/Builders/TestCache.cs
@lahma

lahma commented Feb 27, 2023

Copy link
Copy Markdown
Contributor Author

Thanks for providing this @lahma. I'm a bit tired today, so I cannot determine if we will run into some of the same problems as we did for the other cache - see e.g. #4104 for the solution.

Thanks for the review. At least in this case the usage in in single point so if the attribute fails at first use it should throw at that point. Consecutive calls of course get the cached value but like discussed in one of the threads that kind of stateful attribute usage would be considered invalid.

@OsirisTerje

Copy link
Copy Markdown
Member

@mikkelbu Have you had time to look into this? Would be nice to have it merged in.

Comment thread src/NUnitFramework/framework/Internal/Builders/TestCache.cs Outdated
@SimonCropp

Copy link
Copy Markdown
Contributor

why is this targeting v3. given we are prepping for v4, i would think this could target master?

@SimonCropp
SimonCropp changed the base branch from v3.13-dev to master April 8, 2023 10:32
@SimonCropp
SimonCropp changed the base branch from master to v3.13-dev April 8, 2023 10:32
@lahma
lahma changed the base branch from v3.13-dev to master April 9, 2023 07:38
@lahma
lahma force-pushed the optimize-test-level-attributes branch from d6d25a9 to 9c23365 Compare April 9, 2023 07:38
@lahma

lahma commented Apr 9, 2023

Copy link
Copy Markdown
Contributor Author

I've changed this to target master.

@lahma
lahma force-pushed the optimize-test-level-attributes branch 3 times, most recently from 5e933a8 to 82cc07c Compare April 11, 2023 09:38
@stevenaw

Copy link
Copy Markdown
Member

Thanks for rebasing this and resolving conflicts @lahma . You were too quick for me to ask. 😁

@lahma

lahma commented Apr 11, 2023

Copy link
Copy Markdown
Contributor Author

Now worries it was my pleasure, I like the pace that things are going into master 👍🏻

@manfred-brands manfred-brands left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice work, more caching.

A few minor comments added.

This does change the behaviour of the actions, but only for badly designed ones that don't put re-initialize stuff in the BeforeTest method. More an item to mention in any release notes.

Comment thread src/NUnitFramework/framework/Internal/Builders/TestCache.cs Outdated
Comment thread src/NUnitFramework/framework/Internal/Builders/TestCache.cs Outdated
@lahma
lahma force-pushed the optimize-test-level-attributes branch from d21d969 to 10ae97c Compare April 13, 2023 09:16
@lahma lahma changed the title Add TestCache to improve CompositeWorkItem build speed Add TestMetadataCache to improve CompositeWorkItem build speed Apr 13, 2023
@lahma

lahma commented Apr 13, 2023

Copy link
Copy Markdown
Contributor Author

@manfred-brands I've done the fixes you requested, I squashed to single commit keep cleaner, PR title and commit text now reflect the new name TestMetadataCache.

@manfred-brands manfred-brands left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the quick reaction. Nothing more from me

@OsirisTerje

Copy link
Copy Markdown
Member

Ok, so we can merge @manfred-brands ?

@manfred-brands
manfred-brands merged commit 2d3ce05 into nunit:master Apr 13, 2023
@manfred-brands

Copy link
Copy Markdown
Member

@OsirisTerje I was sitting in the bus when @lahma made his latest changes. Merged now.

@lahma
lahma deleted the optimize-test-level-attributes branch April 13, 2023 10:21
@OsirisTerje

Copy link
Copy Markdown
Member

Awesome!

@manfred-brands manfred-brands mentioned this pull request Mar 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants