-
Notifications
You must be signed in to change notification settings - Fork 789
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
Fix 635: combine boolean logic #5116
Conversation
There are 4 test failures:
|
Yes, the casing of the error message changed: 'the type of' vs 'the Type of' |
|
||
override x.ToString() = |
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.
This is the change that seems to break tests.
the casing of 'type' has changed.
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.
Thanks for spotting this.
I looked at the result, and the instruction layout is not exactly the same than C#, but there is no extra instruction anymore ! Great ! |
Using the following file:
And I get around 2.2 sec for previous version, and 2.7 sec for 'optimized' version, and 2.0 sec for cs version… |
@thinkbeforecoding Could you use BenchmarkDotNet to benchmark you sample instead? |
Using benchmark.net is sensible. A bunch of very controlled profiling tests would be great. There are numerous variables we need to be careful about: processor, 32/64-bit, net core c netfx runtime, /optimize etc. Also branch prediction doesn't make it easy to generalise results. |
@thinkbeforecoding could you post both benchmarks where you see improvements and regressions (and is the one above really a regression? I'll check too) |
I'm working on a better benchmark with benchmarkdotnet |
BenchmarkDotNet is giving: This is testing applying isAlphaNum on the given file... |
@thinkbeforecoding thanks for testing. Please link a zip and give full details of OS, .NET runtime, architecture, runtime etc. thx |
@thinkbeforecoding I couldn't unzip that archive - could you post it with ZIP please? thanks |
@thinkbeforecoding On a quick test I got this on x64:
x32:
Those are just approximate but the numbers are pretty consistent. That's for this code: let isAlphaNum (s:string) pos =
let c = s.[pos]
(c >= '0' && c<='9') || (c>='A' && c<='Z') || (c>='a' && c <= 'z')
[<EntryPoint>]
let main argv =
let txt = System.IO.File.ReadAllText @"c:\GitHub\dsyme\NInterpret\Platforms\NInterpret.Xamarin.Droid\obj\Debug\lp\16\jl\bin\R.txt"
let mutable c = false
let len = txt.Length - 1
let sw = System.Diagnostics.Stopwatch.StartNew()
for _ in 1 .. 10000 do
for i in 0 .. len do
c <- isAlphaNum txt i
sw.Stop()
printfn "%O" sw.Elapsed
0 Compiling and running with
So that looks like speed up to me, but we should continue to test |
Ok, I try to test more cases today 👍🏻
jeremie chassaing / thinkbeforecoding
|
The .net core test on the same sample is giving similar results:
|
More benchmark: BenchmarkDotNet=v0.10.14, OS=Windows 10.0.17134
Intel Core i7-4650U CPU 1.70GHz (Haswell), 1 CPU, 4 logical and 2 physical cores
.NET Core SDK=2.1.300
[Host] : .NET Core 2.1.0 (CoreCLR 4.6.26515.07, CoreFX 4.6.26515.06), 64bit RyuJIT DEBUG
Clr : .NET Framework 4.7.1 (CLR 4.0.30319.42000), 64bit RyuJIT-v4.7.3110.0
Core : .NET Core 2.1.0 (CoreCLR 4.6.26515.07, CoreFX 4.6.26515.06), 64bit RyuJIT
The code: [<CoreJob; ClrJob;GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory);CategoriesColumn>]
type BoolPerf2() =
let txt = System.IO.File.ReadAllText @"visualfsharp\packages\StrawberryPerl64.5.22.2.1\Tools\c\x86_64-w64-mingw32\include\mshtml.h"
let len = txt.Length - 1
[<Benchmark; BenchmarkCategory("AlphaNum")>]
member __.Optimized_AlphaNum() =
for _ in 1 ..3 do
for i in 0 .. len do
TestOptimized.isAlphaNum txt.[i] |> ignore
[<Benchmark; BenchmarkCategory("NumAlpha")>]
member __.Optimized_NumAlpha() =
for _ in 1 ..3 do
for i in 0 .. len do
TestOptimized.isNumAlpha txt.[i] |> ignore
[<Benchmark(Baseline = true); BenchmarkCategory("AlphaNum")>]
member __.Cs_AlphaNum() =
for _ in 1 ..3 do
for i in 0 .. len do
TestCs.isAlphaNum txt.[i] |> ignore
[<Benchmark(Baseline = true); BenchmarkCategory("NumAlpha")>]
member __.Cs_NumAlpha() =
for _ in 1 ..3 do
for i in 0 .. len do
TestCs.isNumAlpha txt.[i] |> ignore
[<Benchmark; BenchmarkCategory("AlphaNum")>]
member __.Original_AlphaNum() =
for _ in 1 ..3 do
for i in 0 .. len do
Test.isAlphaNum txt.[i] |> ignore
[<Benchmark; BenchmarkCategory("NumAlpha")>]
member __.Original_NumAlpha() =
for _ in 1 ..3 do
for i in 0 .. len do
Test.isNumAlpha txt.[i] |> ignore
And the implementation of isAlphaNum and isNumAlpha:
``` F#
let isAlphaNum c =
(c>='a' && c<='z') || (c >= '0' && c<='9')
let isNumAlpha c =
(c >= '0' && c<='9') || (c>='a' && c<='z')
So the optimized version is sometime better than C# version. Depends highly on final code layout and CPU branch prediction based on input data. |
Great ! |
@dsyme, is this ready to go? |
Note to self: baselines need updating:
|
OK, this is ready to go |
Yeah, cool ! Thx
jeremie chassaing / thinkbeforecoding
De : Don Syme
Envoyé le :jeudi 13 septembre 2018 16:37
À : Microsoft/visualfsharp
Cc : Jérémie Chassaing; Mention
Objet :Re: [Microsoft/visualfsharp] Fix 635: combine boolean logic (#5116)
OK, this is ready to go
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
@dsyme …. misery …. your last pr that I just merged stepped over these baselines. |
@dsyme, I hope I resolved them right. |
Maybe off-topic but the title sounds bit like what I did for LINQ-expression trees (and the same could be done for F# Exprs): https://thorium.github.io/Linq.Expression.Optimizer/ |
This fixes #635 to produce better code for boolean logic. It is based on a draft by @thinkbeforecoding. The effect is to remove extra branching in nested
a && b || c
kind of logic, e.g.For a variation of the repro case
The F# code is smaller and basically identical to the equivalent C# except with one branch switched
In earlier examples this provided a benefit for the JITted assembly code as well, at least in size, though I haven't double checked the particular example above.
To be conservative this PR ensures no code duplication occurs, it's a strict simplification. This means that other cases like
isAlphaNumeric
from the linked bug are also a bit smaller but not as small as the equivalent C#. This is because the F# TAST is not fully able to express the linearization of the decision logic in this sort of code without duplication sub-expressions somewhere along the way (or using "goto"):C# fully linearizes this. I don't think we will be able to implement this except via further work at the IlxGen.fs stage, which is independent of this PR.