Optionally bypass structure checks for performance #105
Conversation
src/Argu/PreCompute.fs
Outdated
| let result = System.Collections.Generic.Dictionary<'key, 'value>(s.Length) | ||
| for pair in s do | ||
| result.Add(fst pair, snd pair) | ||
| result :> System.Collections.Generic.IDictionary<_,_> |
There was a problem hiding this comment.
It was to keep the signature the same but as the result is only ever used in private types it's useless. Fixed it.
src/Argu/PreCompute.fs
Outdated
| |> Seq.append helpParam.Flags | ||
| |> Seq.filter (fun name -> name.Length = 2 && name.[0] = '-' && Char.IsLetterOrDigit name.[1]) | ||
| |> Seq.map (fun name -> name.[1]) | ||
| |> System.Linq.Enumerable.Distinct |
There was a problem hiding this comment.
why the Linq distinct function?
There was a problem hiding this comment.
An artefact of an optimization ;)
The F# version ends up initializing some special comparer (For F# types) that references things in a module from FSharp.Core.
When initialized this module (fsharp.core!<StartupCode$FSharp-Core>.$Prim-types..cctor()) loads translation strings for standard exceptions (invalidOp & friends), by searching for dlls on disk and loading them in memory to load resources and this ends up costing ~80ms
In the end i'm not using this function anymore in the "load serialized data" side so I'll change it back (Other functions call into $Prim-types in the main usage, no way to avoid it here)
|
This is awesome work, well done! |
|
BTW here are the perf results with / without checking the structure
Flame graph with Flame graph with |
|
Very visible on the second graph is the problem that is It come from the error strings in Maybe I should PR a change to FSharp.Core here, all other exception generation is made via functions and it's nearly the only case where they are resolved in the static constructor... |
|
PR to FSharp.Core ftw! |
|
@vbfox Cool. I'd be interested to see the numbers compared to the current release of Argu. |
|
Does something speak against setting checkStructure automatically to false for release builds? |
|
@toburger Nothing as long as you test each version in debug mode first, that's what I suggest in http://fsprojects.github.io/Argu/perf.html#Bypassing-structure-checks Other solution is to force the check in unit tests (So if you have them run on PRs you see if a PR introduce something invalid) |
|
@eiriktsarpalis Here is a run on master before my changes :
|
Lazy<T>Note: Some optimizations and additional fields (Like the addition of
GroupedSwitchRegex) can seem to make no sense. They are here to pave the way for the next optimization that will allow to serialize the wholeUnionArgInfostructure and loading it instead of computing it. The corresponding WIP branch can be found here : https://github.com/vbfox/Argu/tree/perf