Skip to content

Introduce MathF.CopySign and Math.CopySign micro benchmarks#1298

Merged
adamsitnik merged 4 commits intodotnet:masterfrom
john-h-k:copysign-benchmarks
Jun 10, 2020
Merged

Introduce MathF.CopySign and Math.CopySign micro benchmarks#1298
adamsitnik merged 4 commits intodotnet:masterfrom
john-h-k:copysign-benchmarks

Conversation

@john-h-k
Copy link
Contributor

@john-h-k john-h-k commented Apr 25, 2020

For dotnet/runtime#35456

Introduce benchmarks for the CopySign methods, which just test with 2500 iters of sign being 0 and 2500 with sign being 1. Criticism welcome as I am not really sure what i am doing here 😃

@john-h-k john-h-k mentioned this pull request Apr 25, 2020
Merged
@billwert
Copy link
Contributor

billwert commented Apr 27, 2020

This looks fine to me. @tannergooding any comments?

tannergooding
tannergooding reviewed Apr 27, 2020
View reviewed changes
src/benchmarks/micro/runtime/Math/Functions/Single/CopySignSingle.cs Outdated
result += MathF.CopySign(result, value);
}

if (value != copySignExpectedResult)
Copy link
Member

@tannergooding tannergooding Apr 27, 2020

Choose a reason for hiding this comment

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

Shouldn't this be result != copySignExpectedResult?

tannergooding
tannergooding reviewed Apr 27, 2020
View reviewed changes
src/benchmarks/micro/runtime/Math/Functions/Single/CopySignSingle.cs
for (int iteration = 0; iteration < MathTests.Iterations; iteration++)
{
value += copySignDelta;
result += MathF.CopySign(result, value);
Copy link
Member

@tannergooding tannergooding Apr 27, 2020

Choose a reason for hiding this comment

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

It might be good if result started off at some non-zero value.... This is going to subtract result 2500 times and then add it back the next 2500
Otherwise we are just testing adding/subtracting +/-0

@tannergooding
Copy link
Member

tannergooding commented Jun 10, 2020

@billwert, I think it looks fine now.

@adamsitnik
Copy link
Member

adamsitnik commented Jun 10, 2020

The CopySign was introduced in .NET Core 3.0 https://apisof.net/catalog/System.Math.CopySign(Double,Double)

The Microbenchmarks.csproj targets older frameworks and because of that the CI fails with following error:

 --------------------------------------------------
 Building .NET micro benchmarks for 'netcoreapp2.1'
 --------------------------------------------------
 $ pushd "/home/helixbot/work/B0E60937/w/B6AC09FC/e/src/benchmarks/micro"
 $ dotnet build /home/helixbot/work/B0E60937/w/B6AC09FC/e/src/benchmarks/micro/MicroBenchmarks.csproj --configuration Release --framework netcoreapp2.1 --no-restore /p:NuGetPackageRoot=/home/helixbot/work/B0E60937/w/B6AC09FC/e/artifacts/packages
 Microsoft (R) Build Engine version 16.2.37902+b5aaefc9f for .NET Core
 Copyright (C) Microsoft Corporation. All rights reserved.
 
   Reporting -> /home/helixbot/work/B0E60937/w/B6AC09FC/e/artifacts/bin/Reporting/Release/netstandard2.0/Reporting.dll
   BenchmarkDotNet.Extensions -> /home/helixbot/work/B0E60937/w/B6AC09FC/e/artifacts/bin/BenchmarkDotNet.Extensions/Release/netstandard2.0/BenchmarkDotNet.Extensions.dll
 runtime/Math/Functions/Single/CopySignSingle.cs(27,33): error CS0117: 'MathF' does not contain a definition for 'CopySign' [/home/helixbot/work/B0E60937/w/B6AC09FC/e/src/benchmarks/micro/MicroBenchmarks.csproj]
 runtime/Math/Functions/Double/CopySignDouble.cs(27,32): error CS0117: 'Math' does not contain a definition for 'CopySign' [/home/helixbot/work/B0E60937/w/B6AC09FC/e/src/benchmarks/micro/MicroBenchmarks.csproj]

@john-h-k could you please extend the following exclusion list with the two new files?

performance/src/benchmarks/micro/MicroBenchmarks.csproj

Lines 73 to 85 in 78ba20f

<ItemGroup Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' Or ('$(TargetFrameworkIdentifier)' == '.NETCoreApp' And '$(_TargetFrameworkVersionWithoutV)' &lt; '3.1')">
<Compile Remove="runtime\Math\Functions\Double\FusedMultiplyAdd.cs" />
<Compile Remove="runtime\Math\Functions\Double\ILogB.cs" />
<Compile Remove="runtime\Math\Functions\Double\Log2.cs" />
<Compile Remove="runtime\Math\Functions\Double\ScaleB.cs" />
<Compile Remove="runtime\Math\Functions\Single\FusedMultiplyAdd.cs" />
<Compile Remove="runtime\Math\Functions\Single\ILogB.cs" />
<Compile Remove="runtime\Math\Functions\Single\Log2.cs" />
<Compile Remove="runtime\Math\Functions\Single\ScaleB.cs" />
<Compile Remove="libraries\System.Memory\SequenceReader.cs" />
<Compile Remove="libraries\System.Numerics.BitOperations\Perf_BitOperations.cs" />
<Compile Remove="libraries\System.Text.Json\**\*.cs" />
</ItemGroup>

it should fix the CI error

adamsitnik
adamsitnik approved these changes Jun 10, 2020
View reviewed changes
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@john-h-k thank you!

@adamsitnik adamsitnik merged commit 2c15891 into dotnet:master Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tannergooding tannergooding tannergooding left review comments

@adamsitnik adamsitnik adamsitnik approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@john-h-k @billwert @tannergooding @adamsitnik