Optimize simplify name analyzer #2254

vasily-kirichenko · 2017-01-14T10:27:19Z

Before

After

(it's the code fix recalculation for 1100 lines file from FSharp.Editing.Tests project from VFPT solution)

It looks like the diagnostics is called twice by VS, I think we should cache the last result in a Dictionary<DocumentId, TextVersion hash * ImmutableArray<Diagnostic>>

add cache

saul · 2017-01-14T11:24:18Z

These speed improvements look great - but over 400 lines of code added with no tests? :(

smoothdeveloper · 2017-01-14T11:36:33Z

vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs

    let getProjectInfoManager (document: Document) = document.Project.Solution.Workspace.Services.GetService<FSharpCheckerWorkspaceService>().ProjectInfoManager
    let getChecker (document: Document) = document.Project.Solution.Workspace.Services.GetService<FSharpCheckerWorkspaceService>().Checker
    let getPlidLength (plid: string list) = (plid |> List.sumBy String.length) + plid.Length
+    static let cache = ConditionalWeakTable<DocumentId, TextVersionHash * ImmutableArray<Diagnostic>>()


Curious if the cache entry deleted when the document is closed?

@smoothdeveloper read what this ConditionalWeakTable is :)

automatically removes the key/value entry as soon as no other references to a key exist outside the table

That's nice, didn't know about that class, thanks for the pointer. So I guess the answer is it doesn't get deleted when document is closed but when it is garbage collected.

vasily-kirichenko · 2017-01-14T12:09:33Z

Added the cache:

vasily-kirichenko · 2017-01-14T12:10:24Z

These speed improvements look great - but over 400 lines of code added with no tests? :(

It's all copied with adjustments, and the code is not used anywhere except this analyzer.

vasily-kirichenko · 2017-01-14T12:42:15Z

forki · 2017-01-14T12:51:47Z

so how much faster is it now?

vasily-kirichenko · 2017-01-14T15:45:38Z

@forki 29 seconds => 800 ms ~= 36 times faster.

forki · 2017-01-14T15:47:39Z

Cool. Now do that to the rest of the compiler :P Am 14.01.2017 4:45 nachm. schrieb "Vasily Kirichenko" < notifications@github.com>:

…

@forki <https://github.com/forki> 29 seconds => 800 ms ~= 36 times faster. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2254 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNMuGxhqhw8aOJ2FMdLNqn-7fY2e3ks5rSO2igaJpZM4Ljlnf> .

vasily-kirichenko · 2017-01-14T15:53:22Z

I've not optimized the compiler, I copied functions and make them fast for my particular case. In short, the existing functions return all items resolvable in a scope, my functions just check that an already resolved item can be resolved being prefixed with a given long ident. It's faster because 1. I pull the available items lazily, until the needed item is found 2. I know the kind of item (ctor, prop, ns or module, etc.) and search only for items of same kind. So these optimizations cannot be used in compiler.

forki · 2017-01-14T15:56:04Z

I got that and was obviously joking. Am 14.01.2017 4:53 nachm. schrieb "Vasily Kirichenko" < notifications@github.com>:

…

I've not optimized the compiler, I copied functions and make them fast for my particular case. In short, the existing functions return all items resolvable in a scope, my functions just check that an already resolved item can be resolved being prefixed with a given long ident. It's faster because 1. I pull the available items lazily, until the needed item is found 2. I know the kind of item (ctor, prop, ns or module, etc.) and search only for items of same kind. So these optimizations cannot be used in compiler. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2254 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNJTuW-bkndIePQG7UPbQDBinivWPks5rSO9ygaJpZM4Ljlnf> .

smoothdeveloper · 2017-01-14T17:43:52Z

@vasily-kirichenko is it wroth to put some comments on the copied functions, to mention the place they are copied from?

I'm thinking that if down the road the original functions change, having that mention would help figure out if catch-up is necessary in those customized functions.

vasily-kirichenko · 2017-01-14T20:51:31Z

More optimization.

before last commit

after

(TypeChecker.fs in FCS solution).

…y line number

KevinRansom · 2017-01-15T03:28:14Z

A few IDE tests failes

Test Run Summary
    Overall result: Failed
   Tests run: 2936, Passed: 2901, Errors: 9, Failures: 26, Inconclusive: 0
     Not run: 99, Invalid: 0, Ignored: 99, Explicit: 0, Skipped: 0
  Start time: 2017-01-14 21:41:17Z
    End time: 2017-01-14 21:57:58Z
    Duration: 1000.828 seconds

forki · 2017-01-15T09:03:19Z

src/fsharp/NameResolution.fs

-        let ctorInfos =  GetIntrinsicConstructorInfosOfType ncenv.InfoReader m typ
-        if isInterfaceTy g typ && isNil ctorInfos then 
+        let ctorInfos = GetIntrinsicConstructorInfosOfType ncenv.InfoReader m typ
+        if isNil ctorInfos && isInterfaceTy g typ then 


Yeah, we should read the whole compiler code to fix such stupid perf penalties.

Reformat ResolveObjectConstructorPrim

vasily-kirichenko · 2017-01-16T13:18:50Z

@dsyme when memory pressure is here, even tooltips can appear with a very long delay, even though code has not changed at all, i.e.:

We occupy 2.5GB
Hover over a symbol - tooltip appears immediately
Hover over another symbol - high CPU load, very long delay and finally we got our tooltip

I think that's because either generating the second tooltip triggered blocking GC or a weak cache data for current file have become available for GC between the first and second tooltips, so the file was rechecked.

dsyme · 2017-01-16T13:37:46Z

@dsyme when memory pressure is here...

My impression is that we must avoid memory pressure > 2GB at all costs, even if some features simply don't work in reasonable time. Once you go above 2GB, nothing works anymore until you restart.

smoothdeveloper · 2017-01-16T13:44:23Z

In my case (with VS2015 + VFPT), I've seen that things may stall or not disregarding the memory occupied by devenv.exe (looking at Working Set (Memory) in task manager).

I've seen process with 2.7gb still doing fine and process with 2gb being unworkable, and I can't really identify why.

vasily-kirichenko · 2017-01-16T13:47:12Z

My impression is that we must avoid memory pressure > 2GB at all costs, even if some features simply don't work in reasonable time.

It's not about features, it's about FCS caches. Features themselves do not add anything to gen 2.

I was thinking about an external process. Maybe it's better to move some FCS internal caches there, not FCS itself, keeping the current VS <-> FCS API untouched.

dsyme · 2017-01-16T14:08:35Z

It's not about features, it's about FCS caches. Features themselves do not add anything to gen 2.

But features do cause information to be required more often. This adds requests to FCS (potentially lengthening the request queue, especially if cancellation is not performed), and may add to compute/allocate/populate/collect churn. The use of a feature can trigger a Gen2 collection in the worst case, while turning off a feature can only improve things.

Also for large files, I believe we reguarly get Gen2 collections even within the typecheck phase of that file itself (even if no caching occurs). So any feature requiring CheckFile results may trigger Gen2.

I'm just concerned we're going to see Gen2 collections or blocking UI pauses triggered by features we can't turn off. When using VFPT I regularly turned off all but a couple of the most important features. Advanced F# programmers don't need to have their code constantly re-analyzed every few seconds for removing "new" or the like.... :) But they do need to have the tools work over 100,000s lines of code :)

Pilchie · 2017-01-17T03:57:36Z

Just as an FYI - a 32-bit process on a 64-bit OS will get a full 4GB of address space, so yes, it's possible to use 4GB of Virtual Memory. Note that things like task manager don't actually report the total VM though - they leave out some categories of memory. To get an accurate picture of VM, use something like vmmap.

For people running 32-bit processes on 32-bit machines, they get either 2 or 3 GB of VM, depending on OS Version and whether the app declares itself large address aware (VS does). The OS always keeps at least 1GB for kernel address space though.

Note however that even with 4GB of address space, you can run into issues with fragmentation on large allocations - the OS (or the CLR) may not have enough contiguous address space to perform an allocation.

Note also that everything I said above was about virtual memory. The amount of physical memory the machine has has no bearing on it - if the machine doesn't have enough physical memory for the virtual memory, some will be swapped to disk. If the machine has more physical memory, it will not be accessible to the process (except through transparent OS optimizations like file-system caching). Address space/VM really come down to pointer-size. A 32-bit process uses 32-bit pointers, which means they can only address 4GB of memory, no matter how much is physically present.

vasily-kirichenko · 2017-01-18T10:41:08Z

We'd better merge it to RTM (because it speeds up the analyzer at about 80 times) or disable it there.

# Conflicts: # vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs

KevinRansom · 2017-02-03T16:56:25Z

@dotnet-bot test this please

KevinRansom

looks good

KevinRansom · 2017-02-03T20:22:50Z

@vasily-kirichenko can you resolve the conflicts pls.

Kevin

…ame-analyzer # Conflicts: # src/fsharp/TypeChecker.fs # src/fsharp/vs/service.fs # src/fsharp/vs/service.fsi

This reverts commit a2f791d. # Conflicts: # src/fsharp/TypeChecker.fs

…copes) by line number" This reverts commit c8edcc3. # Conflicts: # src/fsharp/vs/service.fs # src/fsharp/vs/service.fsi

vasily-kirichenko · 2017-02-11T09:23:17Z

@KevinRansom Could you please merge it? The analyzer is slow and it's still disabled in this PR (as in master) until we have a proper settings dialog to turn it on and off. Keep it up to date with master is cumbersome.

dsyme · 2017-02-17T13:19:22Z

src/fsharp/ast.fs

 // PERFORMANCE: consider making this a struct.
 [<System.Diagnostics.DebuggerDisplay("{idText}")>]
-[<Sealed>]
+[<Struct>]


I'm only now reviewing these changes. Please make sure I'm listed as a reviewer for all changes to the core compiler data structures, just to double check.

Do we know for sure this as a good idea? range is already a largish struct, and we had previously held off making Ident a struct too until we had some data

dsyme · 2017-02-17T13:21:45Z

src/fsharp/tast.fs

    member x.SetIsStructRecordOrUnion b  = let x = x.Data in let flags = x.entity_flags in x.entity_flags <- EntityFlags(flags.IsPrefixDisplay, flags.IsModuleOrNamespace, flags.PreEstablishedHasDefaultConstructor, flags.HasSelfReferentialConstructor, b)

+and [<RequireQualifiedAccess>] MaybeLazy<'T> =
+    | Strict of 'T


This is a good change, thanks. The name is a bit confusing since it might be confused with the Haskell terminology of maybe for option. Also in future please make sure we have /// comments for all new types, methods and properties for these core types.

dsyme · 2017-02-17T13:23:36Z

src/fsharp/tast.fs

      nlr: NonLocalValOrMemberRef }
-    member x.IsLocalRef = match box x.nlr with null -> true | _ -> false
-    member x.IsResolved = match box x.binding with null -> false | _ -> true
+    member x.IsLocalRef = obj.ReferenceEquals(x.nlr, null)


Please double check this change for performance or at least the generated IL

match box x with null -> ... | ... should, AFAIK, generate the best code, and should also allow optimization to brfalse conditiona sitching in the IL. There's no need to remove it from the compiler - if it's not generating good code we should fix our codegen.

dsyme · 2017-02-17T13:24:20Z

src/fsharp/tast.fs

    member vr.Deref = 
-        match box vr.binding with 
-        | null ->
+        if obj.ReferenceEquals(vr.binding, null) then


Again there's no need for this change AFAIK. It's not a big problem either way, it's just that we should only make changes like this if they're needed

dsyme · 2017-02-17T13:24:54Z

src/fsharp/tast.fs


 // Can cpath2 be accessed given a right to access cpath1. That is, is cpath2 a nested type or namespace of cpath1. Note order of arguments.
-let canAccessCompPathFrom (CompPath(scoref1,cpath1)) (CompPath(scoref2,cpath2)) =
+let inline canAccessCompPathFrom (CompPath(scoref1,cpath1)) (CompPath(scoref2,cpath2)) =


Please don't add inline to the compiler unless there's a very specific known and measured perf reason to do it, thanks

dsyme · 2017-02-17T13:25:18Z

src/fsharp/vs/service.fs

    let GetBestEnvForPos cursorPos  =

-        let bestSoFar = ref None
+        let mutable bestSoFar = None


This is a good change

dsyme · 2017-02-17T13:26:32Z

vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs

    let getChecker (document: Document) = document.Project.Solution.Workspace.Services.GetService<FSharpCheckerWorkspaceService>().Checker
    let getPlidLength (plid: string list) = (plid |> List.sumBy String.length) + plid.Length
+    static let cache = ConditionalWeakTable<DocumentId, TextVersionHash * ImmutableArray<Diagnostic>>()
+    static let guard = new SemaphoreSlim(1)


That's this lock for? Please add a big comment on this to explain why we need a lock here, thanks

dsyme · 2017-02-17T13:34:17Z

@vasily-kirichenko Lots of good changes here - I'm only just reviewing them - I hadn't realized there were core changes in the compiler. I added some comments about the changes to the core compiler code

@KevinRansom Probably best you make sure I review changes to the core compiler code. Just for a double check.

The cool thing about making Ident a struct is that there were a lot of such
objects (~1M in the above scenario) and all of them lived in Gen 2, so the GC had to check
them all in memory pressure case.

@vasily-kirichenko Yes, you're likely correct that Ident should be made a struct. However it can be hard to tell for objects that contain range - which is already a struct - and quite a large one for 32-bit systems. Also in many cases Ident objects can I presume be shared. Making them structs can potentially lose that sharing.

BTW I've found it quite hard to measure the pros/cons of these perf choices - the compiler perf scripts aren't yet accurate enough, though the memory tests are fairly accurate. Focusing on reducing memory by using structs can sometimes lead to reduced throughput through lots of data copying.

* remove unnecessary filtering and deduplication * write lazy early return functions for SimplifyName analyzer * make the rest of nr lazy add cache * fix cache locking * do not try to resolve ctors for non-ctor Item * more lazyness to NameResolution * optimize GetBestEnvForPos by indexing nameres environments (scopes) by line number * look for best resolution env on previous line as well * Reformat ResolveObjectConstructorPrim * remove double hash table lookup in NotifyNameResolution * make CurrentSink non-optional * eliminate a list creation * renaming * optimize ValRef * small optimizations * make Ident a struct * add MaybeLazy * SimplifyNameDiagnosticAnalyzer should do semantic analysis, not syntax one * remove dead code * fix after merge * Revert "make CurrentSink non-optional" This reverts commit a2f791d. # Conflicts: # src/fsharp/TypeChecker.fs * Revert "optimize GetBestEnvForPos by indexing nameres environments (scopes) by line number" This reverts commit c8edcc3. # Conflicts: # src/fsharp/vs/service.fs # src/fsharp/vs/service.fsi * turn off SimplifyNameDiagnosticAnalyzer for now

vasily-kirichenko added 2 commits January 14, 2017 10:15

remove unnecessary filtering and deduplication

b3b02b9

write lazy early return functions for SimplifyName analyzer

990b01e

msftclas added the cla-already-signed label Jan 14, 2017

make the rest of nr lazy

0982b92

add cache

smoothdeveloper reviewed Jan 14, 2017

View reviewed changes

fix cache locking

f6d4a7d

do not try to resolve ctors for non-ctor Item

7eeb470

more lazyness to NameResolution

4fc2f9b

optimize GetBestEnvForPos by indexing nameres environments (scopes) b…

c8edcc3

…y line number

vasily-kirichenko force-pushed the optimize-simplify-name-analyzer branch from a80e607 to c8edcc3 Compare January 14, 2017 20:53

vasily-kirichenko and others added 2 commits January 15, 2017 11:26

look for best resolution env on previous line as well

cba5226

Reformat ResolveObjectConstructorPrim

c30aab1

forki reviewed Jan 15, 2017

View reviewed changes

vasily-kirichenko added 5 commits January 15, 2017 12:44

Merge pull request #5 from forki/name-ana

8f92284

Reformat ResolveObjectConstructorPrim

remove double hash table lookup in NotifyNameResolution

9d5db9a

make CurrentSink non-optional

a2f791d

eliminate a list creation

b67938e

renaming

d9501f6

KevinRansom added the Approved for v.Next label Jan 16, 2017

vasily-kirichenko added 3 commits January 21, 2017 15:27

Merge branch 'master' into optimize-simplify-name-analyzer

fe1f967

Merge branch 'master' into optimize-simplify-name-analyzer

e134de1

# Conflicts: # vsintegration/src/FSharp.Editor/Diagnostics/SimplifyNameDiagnosticAnalyzer.fs

remove dead code

2e636c8

KevinRansom approved these changes Feb 3, 2017

View reviewed changes

vasily-kirichenko added 5 commits February 11, 2017 11:05

Merge remote-tracking branch 'origin/master' into optimize-simplify-n…

72124d6

…ame-analyzer # Conflicts: # src/fsharp/TypeChecker.fs # src/fsharp/vs/service.fs # src/fsharp/vs/service.fsi

fix after merge

f750dbb

Revert "make CurrentSink non-optional"

6887a33

This reverts commit a2f791d. # Conflicts: # src/fsharp/TypeChecker.fs

Revert "optimize GetBestEnvForPos by indexing nameres environments (s…

69563dd

…copes) by line number" This reverts commit c8edcc3. # Conflicts: # src/fsharp/vs/service.fs # src/fsharp/vs/service.fsi

turn off SimplifyNameDiagnosticAnalyzer for now

4766680

KevinRansom merged commit 6efa3b3 into dotnet:master Feb 13, 2017

dsyme reviewed Feb 17, 2017

View reviewed changes

Optimize simplify name analyzer #2254

Optimize simplify name analyzer #2254

Uh oh!

Conversation

vasily-kirichenko commented Jan 14, 2017

Uh oh!

saul commented Jan 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasily-kirichenko commented Jan 14, 2017

Uh oh!

vasily-kirichenko commented Jan 14, 2017

Uh oh!

vasily-kirichenko commented Jan 14, 2017

Uh oh!

forki commented Jan 14, 2017

Uh oh!

vasily-kirichenko commented Jan 14, 2017

Uh oh!

forki commented Jan 14, 2017 via email

Uh oh!

vasily-kirichenko commented Jan 14, 2017

Uh oh!

forki commented Jan 14, 2017 via email

Uh oh!

smoothdeveloper commented Jan 14, 2017

Uh oh!

vasily-kirichenko commented Jan 14, 2017

Uh oh!

KevinRansom commented Jan 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vasily-kirichenko commented Jan 16, 2017

Uh oh!

dsyme commented Jan 16, 2017

Uh oh!

smoothdeveloper commented Jan 16, 2017

Uh oh!

vasily-kirichenko commented Jan 16, 2017

Uh oh!

dsyme commented Jan 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Pilchie commented Jan 17, 2017

Uh oh!

vasily-kirichenko commented Jan 18, 2017

Uh oh!

KevinRansom commented Feb 3, 2017

Uh oh!

KevinRansom left a comment

Choose a reason for hiding this comment

Uh oh!

KevinRansom commented Feb 3, 2017

Uh oh!

vasily-kirichenko commented Feb 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dsyme Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

dsyme commented Jan 16, 2017 •

edited

Loading

vasily-kirichenko commented Feb 11, 2017 •

edited

Loading

dsyme Feb 17, 2017 •

edited

Loading