-
Notifications
You must be signed in to change notification settings - Fork 256
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
Add missing tests from .NET Core 3.0 perf tests blog post #478
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
@stephentoub thank you very much for adding all these benchmarks to the performance repo!
Overall LGTM, but I left a few questions.
Could you please rever the renaming changes?
src/benchmarks/micro/corefx/System.Net.Http/Perf.SocketsHttpHandler.cs
Outdated
Show resolved
Hide resolved
public class DnsTests | ||
{ | ||
[Benchmark] | ||
public IPHostEntry GetHostEntry() => Dns.GetHostEntry("34.206.253.53"); |
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.
what is this address pointing to? what are we measuring here?
I want to have a better understanding.
src/benchmarks/micro/corefx/System.Runtime.Extensions/Perf.StreamWriter.cs
Outdated
Show resolved
Hide resolved
src/benchmarks/micro/corefx/System.Runtime.Extensions/Perf.WebUtility.cs
Outdated
Show resolved
Hide resolved
@@ -17,35 +18,45 @@ public class Perf_Int32 | |||
public static IEnumerable<object> Values => new object[] | |||
{ | |||
int.MinValue, | |||
4, // single digit |
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.
should we also add such a test case for uint
, long
and ulong
?
{ | ||
_timers[i] = new Timer(_ => { }, null, i, i); | ||
} | ||
Thread.Sleep(1000); |
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.
nit: it would be nice to add a comment explaining why we need this sleep here
# Conflicts: # src/benchmarks/micro/corefx/System.Linq/Perf.Linq.cs # src/benchmarks/micro/corefx/System.Runtime/Perf.Guid.cs
… solving merge conflict
@stephentoub I am currently doing the 2.2 vs 3.0 comparison and I really want to include these benchmarks in my results. To save some time I've just solved the merged conflict in your branch and fixed the few issues that I've found during the review. I hope you don't mind. |
That's fine, thanks. |
It looks like this change is causing build failures in the the lab. See: https://dev.azure.com/dnceng/internal/_build/results?buildId=270482 What I am seeing is this:
Which leads to failures when we try to write measurement.json later:
How necessary is the WriteFormat perf test, and can we back out that single test to get the lab clean? |
@adiaaida I am going to send a fix in a few minutes |
Great, thank you. |
Hm. I think CI would have caught this, but I only see the WIP check. We should understand where the rest went. |
AzDO was down yesterday afternoon/evening for PRs (they weren't triggering) |
Ah, got it. |
https://devblogs.microsoft.com/dotnet/performance-improvements-in-net-core-3-0/
cc: @adamsitnik, @billwert