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

Perf: create a tracker per module and use indexes to improve performance #181

Merged

Conversation

pjanotti
Copy link
Contributor

@pjanotti pjanotti commented Aug 22, 2018

This change came from the discusson on PR #172.

This change brings the performance for System.Text.Encondings.Web.Tests in CoreFx from around 113 seconds to around 19 seconds. The coverlet performance test project now runs for 20,000 iterations in about the same time that it previously was running for 200 iterations, now at 200 iterations the time is dominated by instrumentation and report generation.

The idea of the change is that by creating a distinct static tracker type per module the dictionaries used before to track execution can be replaced with an array to represent the hit locations. This array is written to the disk in binary format, only the number of hits per index, avoiding the need to parse to process the hit files. The trade-off is that this should represent a bit more consumption of memory for projects with very low coverage numbers.

@pjanotti
Copy link
Contributor Author

@tonerdo I know that you just merged #172 but I got this PR ready 😀

I will be slow to answer any Q/comments for a few days, so no rush with this one. It will be great if other contributors could spin this PR against their projects just to be sure that there isn't some corner case missed here.

/cc @tarekgh @petli

@pjanotti
Copy link
Contributor Author

Side note: this dosen't solve instrumentation of System.Private.CoreLib, but gets a path to a possible fix: when instrumenting corelib/mscorlib instead of generating the tracker code as

newobj instance void class [netstandard]System.Collections.Generic.List`1<int32[]>::.ctor(int32)

we need to generate

newobj instance void class System.Collections.Generic.List`1<int32[]>::.ctor(int32)

(see https://github.com/dotnet/corefx/issues/31281)

@petli
Copy link
Collaborator

petli commented Aug 24, 2018

Testing this PR against one of the bigger unit test projects in the code I'm working on at the day job the performance improvements are very good:

Test type Test execution time Total time to run dotnet test --no-build
Without coverage 30s 31s
coverlet 2.2.1 133s 144s
PR changes 34s 37s

Impressive, @pjanotti!

@@ -14,8 +14,9 @@ namespace coverlet.core.performancetest
/// </summary>
public class PerformanceTest
{
[Theory(Skip = "Only enabled when explicitly testing performance.")]
[InlineData(150)]
[Theory(/*Skip = "Only enabled when explicitly testing performance."*/)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the test be enabled by default now, or remain Skip in the repo code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The perf test is not run by default so we don't need the skip in place (avoids us changing the source just to test perf). That said I should have removed the skip instead of commenting it, I'll clean it up on my next change.

foreach (TypeDefinition type in types)
{
var actualType = type.DeclaringType ?? type;
if (!actualType.CustomAttributes.Any(IsExcludeAttribute)
&& actualType.Namespace != "Coverlet.Core.Instrumentation.Tracker"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not add an ExcludeFromCodeCoverage attribute to all the methods in that class?

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 see that you already removed it 👍 Thanks, it was not needed as you noticed

Copy link
Collaborator

@tonerdo tonerdo left a comment

Choose a reason for hiding this comment

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

This is great work @pjanotti and it's very much appreciated! I fully understand your approach and I don't see anything that jumps out at me. I don't have a lot of multithreading experience so I'll defer to @petli or @tarekgh to sign off on this for merging

Copy link
Collaborator

@petli petli left a comment

Choose a reason for hiding this comment

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

Threading parts look good to me, @tonerdo

@tonerdo tonerdo merged commit 5a032f5 into coverlet-coverage:master Sep 1, 2018
@pjanotti
Copy link
Contributor Author

pjanotti commented Sep 6, 2018

@tonerdo and @petli thanks for reviewing this PR. I will address the comments soon.

@pjanotti pjanotti deleted the tracker.type.per.module.rebase branch September 6, 2018 18:10
NorekZ pushed a commit to NorekZ/coverlet that referenced this pull request Nov 8, 2018
…er.module.rebase

Perf: create a tracker per module and use indexes to improve performance
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants