-
Notifications
You must be signed in to change notification settings - Fork 288
Add tests for new Random API and for the distinct unseeded path #1637
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
Conversation
|
BenchmarkDotNet=v0.12.1.1466-nightly, OS=Windows 10.0.19042 PowerPlanMode=00000000-0000-0000-0000-000000000000 Arguments=/p:DebugType=portable IterationTime=250.0000 ms
|
|
Couple of nits:
|
Yes, this is because I want to avoid changing benchmark names, so we don't break history. And unfortunately the ctor was the exception in that it measured unseeded. (@adamsitnik I assume there isn't an attribute that would allow me to have consistent method names while still being recorded under the old test name...)
I'll add. |
src/benchmarks/micro/libraries/System.Runtime.Extensions/Perf.Random.cs
Outdated
Show resolved
Hide resolved
|
OK, should be done now. I added a bunch of #if's for new API since it seemed more readable in this case than separate files which is what we mostly seem to do in this repo. |
|
@billwert some CI legs are failing with |
|
It looks like the actual failure was that installation of the SDK was not reliable this afternoon. (For example). These are probably fine to ignore. I've got a work item to fix this up. |
|
@stephentoub any more feedback ? As noted above, I can change to 3 partial classes, but I think this is one of those cases where #if actually makes it much clearer what's going on. |
|
Thanks @billwert.. would retrying help or not? |
I'll defer to @adamsitnik. I prefer the partial files approach, but if the preferred scheme for this repo is ifdefs, that's fine. |
src/benchmarks/micro/libraries/System.Runtime.Extensions/Perf.Random.cs
Outdated
Show resolved
Hide resolved
|
Merging with CI failures due to missing SDK on some machines, noted by @billwert @adamsitnik if you believe partial classes would be clearer, LMK and I'll put up a new PR. |
You are right. Perhaps we should create such an attribute?
Great! @danmosemsft big thanks for adding the benchmarks! @colgreen very nice improvements! |
Tests for changes in dotnet/runtime#47390
cc @colgreen