Change release builds to be optimized #4350

lahma · 2023-04-13T11:11:34Z

As discussed in #4149 , seems like a good time with 4.0 nearing to try out again distributing optimized release builds. With this PR removing the forced no-optimize and BenchmarkDotNet project proves that artifacts can now be consumed as release builds (no longer need to disable validator).

Quick benchmark test for changes. Shows > 10% time reduction in best cases (very feature-limited benchmarks of course).

BenchmarkDotNet=v0.13.5, OS=Windows 11 (10.0.22621.1555/22H2/2022Update/SunValley2)
12th Gen Intel Core i9-12900H, 1 CPU, 20 logical and 14 physical cores
.NET SDK=7.0.203
  [Host] : .NET 6.0.16 (6.0.1623.17311), X64 RyuJIT AVX2

Job=InProcess  Toolchain=InProcessEmitToolchain

Before (master)

Method	Mean	Error	StdDev	Gen0	Gen1	Gen2	Allocated
CountTestsEmptyFilter	138.5 us	1.89 us	1.77 us	4.1504	-	-	51.6 KB
CountTestsWithCategoryFilter	2,758.3 us	52.83 us	49.41 us	500.0000	-	-	6172.07 KB
CountTestsWithOrFilterAll	4,806.7 us	58.17 us	51.56 us	312.5000	125.0000	23.4375	3903.07 KB
LoadTests	66,056.0 us	1,224.24 us	1,202.37 us	2125.0000	875.0000	250.0000	24510.9 KB

After (this PR)

Method	Mean	Error	StdDev	Gen0	Gen1	Gen2	Allocated
CountTestsEmptyFilter	116.8 us	1.72 us	1.61 us	4.1504	-	-	51.6 KB
CountTestsWithCategoryFilter	2,332.5 us	28.56 us	26.72 us	500.0000	-	-	6172.07 KB
CountTestsWithOrFilterAll	3,921.2 us	76.52 us	71.58 us	312.5000	125.0000	23.4375	3903.08 KB
LoadTests	61,599.0 us	1,057.11 us	988.82 us	2222.2222	1000.0000	333.3333	24131.66 KB

manfred-brands

You are on a roll.
Did you check how it affects end user debugging?

lahma · 2023-04-13T11:34:11Z

I had to change to draft as there seems to be some tests checking that optimizations are off.

You are on a roll. Did you check how it affects end user debugging?

As discussed in original issue, I would assume debugging experience is the same for NUnit developers when developing in debug mode (IDE setting). I don't see a normal use case of consumers of the NUnit library for debugging say, why Assert behaves they way it does that often. I myself am accustomed to stepping into 3rd party code and if optimized, it's "normal" for debugger stepping behaving a bit wonky. IDEs usually download the symbols etc and use source link to get the code in question.

manfred-brands · 2023-04-13T11:46:12Z

@lahma Any idea why the build failed?

lahma · 2023-04-13T12:07:14Z

@manfred-brands I believe the reason was just this assert here https://github.com/nunit/nunit/blob/master/src/NUnitFramework/tests/TestDataAssemblyTests.cs . So I changed test data projects not be to be optimized even if Release build set (like in CI).

lahma · 2023-04-14T06:03:50Z

So test projects can also be set to Optimize=true (the default now). Had to fix two things:

SlowPath_TakenWhenSpecialTypes verifies code path selection via stopwatch (lovely), so lowered the value from 50ms to 30ms as optimized code is fast(!)
DefaultConstraintTests.Success_DefaultStructWithOverriddenEquals() has different ToString on full framework for some reason under optimized build but this is the behavior I believe NUnit consumer will see anyway because the type in question will be optimized based on their project configuration

stevenaw · 2023-04-18T17:11:54Z

src/NUnitFramework/tests/Constraints/DefaultConstraintTests.cs

@@ -20,8 +20,10 @@ public DefaultConstraintTests()
        [Test] public void Success_Int() => AssertSuccess(default(int));
        [Test] public void Success_NullableInt() => AssertSuccess(default(int?));
        [Test] public void Success_Long() => AssertSuccess(default(long));
+#if !NETFRAMEWORK // the error message is different between class framework and newer .NET runtimes with optimized builds


Curious. Do you recall how it's different? I could see stack traces changes, but I'm not sure what else could.
I'm a little concerned that this loses some test coverage on netfx runs. Is there a way we could keep the coverage of the the DefaultStructWithOverriddenEquals scenario here on all builds?

Oops, I see now that you explained the difference. I'll have to take a closer look here. I'm still curious if we can avoid losing the netfx coverage here

I was a bit surprised about this too but I would believe this is quite the corner case, I don't think it will much difference when deciphering a failed Assert in test log. As said, I would believe they were already getting the same message if they were using optimized code against the assert.

I've pulled your branch and this test seems to pass for me locally on all platforms, including net462 if I remove the conditional compilation. Does it matter if the ToString() differs?

@stevenaw here's the error if the assert is removed: https://github.com/nunit/nunit/actions/runs/4739738110/jobs/8414851760?pr=4350#step:8:423

Errors, Failures and Warnings 1) Failed : NUnit.Framework.Constraints.DefaultConstraintTests.Success_DefaultStructWithOverriddenEquals Expected: default But was: NUnit.Framework.Constraints.DefaultConstraintTests+StructWithStrangeEquals at NUnit.Framework.Constraints.DefaultConstraintTests.AssertSuccess[T](T actual) at NUnit.Framework.Constraints.DefaultConstraintTests.Success_DefaultStructWithOverriddenEquals() Run Settings Number of Test Workers: 2 Work Directory: D:\a\nunit\nunit\src\NUnitFramework\tests\bin\Release\net462 Internal Trace: Off

It also fails for me locally, I pushed back the version that has the precompilation directive.

I have upgraded my SDK to the same as the build server used, but it is still not failing for me.
This is scary, code that works only on some machines.
@stevenaw Does this test fail for you?

I missed it earlier but @stevenaw already said the tests worked for him as well.

I suggest to remove the test and the bad struct.

I removed the test case but kept the struct as it's referenced in equality test that isn't broken. I force pushed the version - not sure how you want your history but single commit probably cleaner as I guess you don't squash merge commits by default.

That test case is covered by the ImmutableArray case and the weird Equatable implementation will confuse any reader. Please remove it.

Yes, single commit is preferred by me

Done and force-pushed.

* set test data projects not be be optimized as non-optimized behavior is expected

manfred-brands · 2023-04-19T13:05:02Z

Thanks @lahma

manfred-brands approved these changes Apr 13, 2023

View reviewed changes

lahma marked this pull request as draft April 13, 2023 11:30

lahma force-pushed the optimize-true branch from 71b8546 to 1946f58 Compare April 13, 2023 11:42

lahma force-pushed the optimize-true branch from 1946f58 to c65d1cf Compare April 13, 2023 12:00

lahma marked this pull request as ready for review April 13, 2023 12:14

lahma force-pushed the optimize-true branch from 143448a to 3374cd9 Compare April 14, 2023 05:56

stevenaw reviewed Apr 18, 2023

View reviewed changes

lahma force-pushed the optimize-true branch from d28ed40 to 700c063 Compare April 19, 2023 05:21

manfred-brands force-pushed the optimize-true branch from 672ca7a to 700c063 Compare April 19, 2023 08:45

lahma force-pushed the optimize-true branch from 700c063 to cfe4e78 Compare April 19, 2023 12:06

Change release builds to be optimized by default

8ea9c57

* set test data projects not be be optimized as non-optimized behavior is expected

lahma force-pushed the optimize-true branch from cfe4e78 to 8ea9c57 Compare April 19, 2023 12:49

manfred-brands merged commit 40b402b into nunit:master Apr 19, 2023

lahma deleted the optimize-true branch April 19, 2023 13:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change release builds to be optimized #4350

Change release builds to be optimized #4350

lahma commented Apr 13, 2023

manfred-brands left a comment

lahma commented Apr 13, 2023

manfred-brands commented Apr 13, 2023

lahma commented Apr 13, 2023 •

edited

Loading

lahma commented Apr 14, 2023 •

edited

Loading

stevenaw Apr 18, 2023 •

edited

Loading

stevenaw Apr 18, 2023

lahma Apr 18, 2023

stevenaw Apr 19, 2023

lahma Apr 19, 2023 •

edited

Loading

manfred-brands Apr 19, 2023

manfred-brands Apr 19, 2023

lahma Apr 19, 2023

manfred-brands Apr 19, 2023

lahma Apr 19, 2023

manfred-brands commented Apr 19, 2023

Change release builds to be optimized #4350

Change release builds to be optimized #4350

Conversation

lahma commented Apr 13, 2023

Before (master)

After (this PR)

manfred-brands left a comment

Choose a reason for hiding this comment

lahma commented Apr 13, 2023

manfred-brands commented Apr 13, 2023

lahma commented Apr 13, 2023 • edited Loading

lahma commented Apr 14, 2023 • edited Loading

stevenaw Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lahma Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manfred-brands commented Apr 19, 2023

lahma commented Apr 13, 2023 •

edited

Loading

lahma commented Apr 14, 2023 •

edited

Loading

stevenaw Apr 18, 2023 •

edited

Loading

lahma Apr 19, 2023 •

edited

Loading