Streamlining and minor cleanup of code in the PrettyNaming module #578
Conversation
|
Hi @jack-pappas, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
|
Do I need to sign the CLA again? I thought I'd already filed this one. |
|
@jack-pappas there was a change of the CLA process a couple of months ago. the new one is much faster/easier to sign and doesn't need employers approval. did you sign this one? |
| let private decompileCustomOpName = | ||
| // Memoize this operation. Custom operators are typically used more than once | ||
| // so this avoids repeating decompilation. | ||
| let decompiledOperators = Dictionary<_,_> (System.StringComparer.Ordinal) |
There was a problem hiding this comment.
Please put this as a private module-level definition and mark it with // +++ GLOBAL STATE
There was a problem hiding this comment.
Any reason not to use ConcurentDictionary? I'd prefer that unless there's a proven case against it.
|
I've taken a close look and it all seems like reasonable cleanup. But I'd appreciate someone else checking the code closely too. |
|
I've left some comments too. |
|
@jack-pappas, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
| @@ -293,7 +417,7 @@ module internal Microsoft.FSharp.Compiler.PrettyNaming | |||
| res <- res && n.[i] >= '0' && n.[i] <= '9'; | |||
| res | |||
|
|
|||
| type NameArityPair = NameArityPair of string*int | |||
| type NameArityPair = NameArityPair of name:string * arity:int | |||
There was a problem hiding this comment.
Named DU fields is a 3.1 feature, but our internal build uses a pre-3.1 compiler to bootstrap. So for now we can't do this.
|
@dsyme I've made some of your requested changes and updated this PR (I'll squash them before the PR is merged). I wasn't able to make I tried modifying the code to use |
|
@latkin I've removed the named fields and updated the code. |
| if s.StartsWith("get_", System.StringComparison.Ordinal) || | ||
| s.StartsWith("set_", System.StringComparison.Ordinal) | ||
| then Some (s.Substring(4, s.Length - 4)) | ||
| else None |
There was a problem hiding this comment.
Can we simplify this further, so there is only a single check for "get_" and "set_" prefixes?
/// Try to chop "get_" or "set_" from a string
let TryChopPropertyName (s: string) =
// extract the logical name from any mangled name produced by MakeMemberDataAndMangledNameForMemberVal
if s.Length <= 4 then None else
let s = chopStringTo s '.'
if s.StartsWith("get_", System.StringComparison.Ordinal) ||
s.StartsWith("set_", System.StringComparison.Ordinal)
then Some (s.Substring(4, s.Length - 4))
else NoneThere was a problem hiding this comment.
I was thinking about making this change when doing the cleanup. It's a good change, I'll make it now and update the PR.
|
Overall LGTM, just some minor suggestions on improving clarity |
|
@dsyme do you have a style preference for this repo regarding .NET method calls: with/without parens? e.g. |
|
@latkin There is no strict preference, but more recent code tends to use the former, so it is ok to move code in that direction. |
jack-pappas
force-pushed
the
prettyname-cleanup
branch
from
43951ae to
309f0b4
Compare
|
@latkin I cleaned up the loop in the One other thing I noticed was that there seemed to be more GC's reported in the I've squashed the commits so this PR can be merged whenever you're ready. |
|
@jack-pappas Great! Would be interesting to find out what's up with the string loop, but no worries in the meantime. I'll try to get this merged soon. |
| System.Text.StringBuilder (maxPossibleOpCharCount) | ||
|
|
||
| // Start decompiling just after the operator prefix. | ||
| match decompile sb opNamePrefixLen with |
There was a problem hiding this comment.
My thought was to have this simply return string directly - instead of returning Some(sb.ToString()) just return sb.ToString(), instead of None return opName
There was a problem hiding this comment.
@latkin Good point. I've made the change and updated the PR.
Compilation and decompilation of operators is now memoized for custom (non-built-in) operators to improve performance. Lifted creation of lists (now arrays) in the IsInfixOperator function so they aren't re-created (potentially) on each function call. Simplification to the TryChopPropertyName function per @latkin's suggestion. Optimized the recursive function which performs the decompilation within 'decompileCustomOpName' so it's tail recursive and avoids allocating substrings and some Option<_> instances.
jack-pappas
force-pushed
the
prettyname-cleanup
branch
from
309f0b4 to
cf72133
Compare
|
|
||
| // extract the logical name from any mangled name produced by MakeMemberDataAndMangledNameForMemberVal | ||
| if s.Length <= 4 then None else | ||
| let s = chopStringTo s '.' |
There was a problem hiding this comment.
Apologies, my earlier suggestion to use this simplification was bogus. This now fails/causes crash for properties with . in them (e.g. backtick identifiers). I'll fix before merging...
|
Looks like you are actively updating this. Let me know when it's done so I can sync locally and re-run tests. |
|
@latkin I'm finished making changes if you're going to fix that one piece of code before merging. Or, I can fix it and update the PR -- just let me know. |
|
Got it, re-running now. I'll take care of the TryChopPropertyName thing, sorry about that. |
closes #578 commit 7e1c8410a3df45df62830f1dd29df18ac89b69a6 Author: latkin <latkin@microsoft.com> Date: Wed Aug 12 14:58:45 2015 -0700 Fix logic in TryChopPropertyName commit cf72133 Author: Jack Pappas <jack-pappas@users.noreply.github.com> Date: Sun Aug 9 15:54:28 2015 -0400 Streamlining and minor cleanup of code in the PrettyNaming module. Compilation and decompilation of operators is now memoized for custom (non-built-in) operators to improve performance. Lifted creation of lists (now arrays) in the IsInfixOperator function so they aren't re-created (potentially) on each function call. Simplification to the TryChopPropertyName function per @latkin's suggestion. Optimized the recursive function which performs the decompilation within 'decompileCustomOpName' so it's tail recursive and avoids allocating substrings and some Option<_> instances.
|
Applied to OOB branch |
I've made some small changes to the PrettyNaming module to streamline it for performance. Specifically:
IsInfixOperatorfunction was creating (potentially) multiple lists each time it was called. I've changed the lists to arrays since they're never modified, and I've lifted these arrays outside of the function body so they're only created once and re-used.if-then-elseexpressions have been changed to usematchinstead so they can be compiled into aswitchopcode when appropriate..Substring(...)operation; they have been changed to use non-allocating comparisons instead (e.g.,.StartsWith(...)).IsValidPrefixOperatorUse,IsValidPrefixOperatorDefinitionName, andIsPrefixOperatorfunctions would, in certain specific cases, allocate a closure. It doesn't appear that this is actually necessary though -- a small modification to the code eliminates the variable capture that causes the allocation, allowing the lambda to be lifted out of the functions.I've done some rudimentary benchmarking, and this change appears to have no significant impact (better or worse) when I compile these changes into a new
fsc-protoand use that to rebuild theFSharp.Compiler.dll. However, these changes do appear to improve compile times by a small but consistent amount when I use thefsc-prototo compile the various flavors ofFSharp.Core. Based on the data produced by--times, these improvements appear to come from reducing the number of Gen0 and Gen1 collections during the[Parse inputs]and[Typecheck]compilation phases.These modifications should not cause any externally-visible differences in behavior (i.e., from the perspective of callers in other parts of the compiler).