Avoid attributes recomputation #109
Conversation
src/Argu/PreCompute.fs
Outdated
| [<AutoOpen>] | ||
| module private FastAttributes = | ||
| let inline hasAttribute<'T when 'T :> Attribute> (attributes: obj[]) = | ||
| attributes |> Array.tryFindIndex (fun x -> x :? 'T) <> None |
There was a problem hiding this comment.
Shouldn't Array.exists suffice here?
There was a problem hiding this comment.
(I need to find a way to remember the name of this method, each time I stay on contains but it's not what I want and I end up rewriting it by other means)
src/Argu/PreCompute.fs
Outdated
| attributes |> Array.tryFindIndex (fun x -> x :? 'T) <> None | ||
|
|
||
| let inline hasAttribute2<'T when 'T :> Attribute> (attributes1: obj[]) (attributes2: Attribute[]) = | ||
| (hasAttribute<'T> attributes1) || (attributes2 |> Array.tryFindIndex (fun x -> x :? 'T) <> None) |
src/Argu/PreCompute.fs
Outdated
| (hasAttribute<'T> attributes1) || (attributes2 |> Array.tryFindIndex (fun x -> x :? 'T) <> None) | ||
|
|
||
| let inline tryGetAttribute<'T when 'T :> Attribute> (attributes: obj[]) = | ||
| attributes |> Array.tryFind (fun x -> x :? 'T) |> Option.map (fun x -> x :?> 'T) |
There was a problem hiding this comment.
Replace with Array.tryPick
There was a problem hiding this comment.
Array.tryPick (function :? 'T as t -> Some t | _ -> None)
src/Argu/PreCompute.fs
Outdated
| let inline tryGetAttribute<'T when 'T :> Attribute> (attributes: obj[]) = | ||
| attributes |> Array.tryFind (fun x -> x :? 'T) |> Option.map (fun x -> x :?> 'T) | ||
|
|
||
| let inline tryGetAttribute2<'T when 'T :> Attribute> (attributes: obj[]) (attributes2: Attribute[]) = |
There was a problem hiding this comment.
Please use System.Attribute as the type for the first input array. I'm assuming the two arguments are type-level vs. case level attributes? Naming of arguments should convey this.
There was a problem hiding this comment.
I'll rename them. But I can't change the type of the first argument, it's what UnionCaseInfo.GetCustomAttributes returns.
There was a problem hiding this comment.
Pretty sure you could map the array though.
There was a problem hiding this comment.
Yes, everything in it is an Attribute, but why reallocate a second array with the exact same data to end up casting it out anyway ? There is no consumer that uses the fact that they are instances of Attribute.
In fact if it's ok with you i'll find what makes the second array contains Attribute and use obj instead. That's what https://msdn.microsoft.com/en-us/library/kff8s254(v=vs.110).aspx returns and any conversion is unneeded
src/Argu/PreCompute.fs
Outdated
| | _ when Option.isSome appSettingsName.Value -> appSettingsName.Value.Value | ||
| | _ -> arguExn "parameter '%O' needs to have at least one parse source." uci) | ||
| | _ -> raise <| new ArguException("parameter '" + (uci.ToString()) + "' needs to have at least one parse source.")) |
There was a problem hiding this comment.
I'm not a fan of this change, premature optimization where raised exception would be the dominating factor. Please revert.
There was a problem hiding this comment.
Ah yes I need to measure, I changed all the printf initialization but I don't think this one is hit.
src/Argu/PreCompute.fs
Outdated
| uci.GetAttributes<AltCommandLineAttribute>() | ||
| |> Seq.collect (fun attr -> attr.Names) | ||
| attributes.Value | ||
| |> Array.filter (fun x -> x :? AltCommandLineAttribute) |
There was a problem hiding this comment.
Could be inlined into a single Array.collect
Here is another performance PR.
The main performance gain is the removal of repeated calls to
GetAttributesand instead doing a singleGetAttributesand reusing the values.I also removed some usage of printf as building the printers take time and it's not very necessary here.
Performance looks like that:
It can be compared to #105 (comment) so the gain is of ~35% with structure check and ~10% without.
And from that point we're fast enough that dotnet/fsharp#4465 will gain 40% for checked and 50% for no checks, bringing the no check version of add nuget Foo.Bar around 150ms :