Better handling of typing installer events and consuming typing files in tsserver by sheetalkamat · Pull Request #23438 · microsoft/TypeScript

sheetalkamat · 2018-04-16T19:01:17Z

Removes unnecessary indirection of unresolved imports map
Force new typings resolution only if there are more or less script infos in the project. Earlier if program is not completely reused, we would enqueue new typing request, which is too frequent and not necessary. The unresolved imports and compiler options change is anyways detected to determine to queue typing request, so we want to force the typing request only if there are more or less source files in the program.
In update graph, do not re-update/create new program but rather just enqueue typing request. We would handle the typing files when there are events of setting typing files from typing installer.
When ActionSet event is received from typing installer, invalidate only files with unresolved imports and resolve those. The invalidation should be quite fast since it just passes list of files with unresolved imports and re-resolution happens during program creation.
When ActionInvalidate event is received, typing installer has detected some change in global typing cache location, so just enqueue a new typing installation request. This will repeat the cycle of setting correct typings and picking unresolved imports. Earlier we use to delete existing typing files and updated program. This meant there would be need to create new program with few files (all files - typing files) and then re-queuing the request and that would just be unnecessary step.
Note: this does not change any file/directory watches in typing installer. That needs to be handles irrespective of these changes.

…rection and helps only in test only

…fos in the project. This helps in reducing number of forced typing installation requests We anyways use changes in unresolved import array to determine if we need to enqueue new typing request Hence there is no need to soley rely on hasChanges from updateGraph which just indicates that we didnt reused the program (that does not mean new files were added to the program or changes in unresolved imports)

This has 3 changes: 1. In updateGraph when enqueue the typing installation request (depending on unresolved imports) 2. When ActionSet event is received, invalidate only files with unresolved imports and resolve those. 3. When ActionInvalidate event is received, typing installer has detected some change in global typing cache location, so just enqueue a new typing installation request. This will repeat the cycle of setting correct typings and pickiing unresolved imports

…since watchers arent closed if they need to be reopened

mhegazy · 2018-04-17T19:29:16Z

    /** Remove the *first* occurrence of `item` from the array. */
-    export function unorderedRemoveItem<T>(array: T[], item: T): void {
+    export function unorderedRemoveItem<T>(array: T[], item: T) {
        unorderedRemoveFirstItemWhere(array, element => element === item);


this still does not return a value.. did you mean to change this to return unorderedRemoveFirstItemWhere(array, element => element === item);

mhegazy · 2018-04-17T19:35:55Z

+        /*@internal*/
+        lastCachedUnresolvedImportsList: SortedReadonlyArray<string>;
+        /*@internal*/
+        hasMoreOrLessScriptInfos = false;


consider renaming this to hasAddedOrRemovedFiles

how is that different from is this the same as projectStructureVersion change?

projectStructureVersion is actually program version = every time new program is created this version is incremented. We could create new program using existing files (without any additional or removal of script infos when we decide its safe to reuse the modules, eg. adding import in file such that imported module was already part of the program, so it would be new program but no additional files woult be there in the program)

mhegazy · 2018-04-17T19:40:55Z

                // found cached result - use it and return
                for (const f of cached) {
-                    result.push(f);
+                    (result || (result = [])).push(f);


kinda weird that we take result as an input and return it as an output.. i would just do one of them..

mhegazy · 2018-04-17T19:43:25Z

+        /**
+         * This is the set that has entry to true if file doesnt contain any unresolved import
+         */
+        private filesWithNoUnresolvedImports = createMap<true>();


why do we need a separate list, would not an empty array or even undefined be sufficient in cachedUnresolvedImportsPerFile?

Agreed. This was before when i was experimenting with the invalidation in a different way.

mhegazy · 2018-04-17T19:45:48Z


-            let hasChanges = this.updateGraphWorker();
+            const hasChanges = this.updateGraphWorker();
+            const hasMoreOrLessScriptInfos = this.hasMoreOrLessScriptInfos;


not sure i understand the hasChanges vs hasMoreOrLessScriptInfos distinction here..

mhegazy · 2018-04-17T19:47:55Z

            const isNew = !this.isAttached(project);
            if (isNew) {
                this.containingProjects.push(project);
+                project.hasMoreOrLessScriptInfos = true;


i would rather make this a function call and let the project handle it by setting the flag, instead of setting the flag here.. just an encapsulation comment.

mhegazy · 2018-04-17T19:49:22Z

    };

-    class TypingsCacheEntry {
+    interface TypingsCacheEntry {


so why did we make this change?

we were never doing new TypingsCacheEntry() so i seems like a interface rather than class

mhegazy · 2018-04-17T19:51:22Z

                this.projectWatchers.set(projectName, watchers);
            }

+            watchers.isInvoked = false;


so were we having same watchers in multiple projects? i am not sure i understand the change here..

Previously the isInvoked was local variable that was captured. If there were additional watches added to the project (because it has more files etc) the new isInvoked would be used for additional watchers, but the existing watches would still refer to the previous isInvoked thus invalidating the typing multiple times or ignoring the typings assuming it was already invalidating.

mhegazy · 2018-04-17T22:36:17Z

        }

+        /* @internal */
+        setHasMoreOrLessFiles() {


consider renaming the function to onFileAddedOrRemoved and the property to hasAddedorRemovedFiles, since the count can be the same, but we still need to invalidate the cache.

sheetalkamat added 5 commits April 13, 2018 15:17

Typings cache is internal data structure

b0fb73c

Remove the specialized type UnresolvedImportsMap which is just a redi…

c9479f7

…rection and helps only in test only

Invoked should be property on watchers map instead of local variable …

82e9a75

…since watchers arent closed if they need to be reopened

mhegazy reviewed Apr 17, 2018

View reviewed changes

Update to respond to PR feedback

d64f248

mhegazy reviewed Apr 17, 2018

View reviewed changes

mhegazy approved these changes Apr 17, 2018

View reviewed changes

Renames as per PR feedback

7c5f524

sheetalkamat merged commit b0d6896 into master Apr 18, 2018

sheetalkamat deleted the typingsFiles branch April 18, 2018 18:35

sheetalkamat mentioned this pull request Apr 18, 2018

Use watch recursive directories instead of watchFile for node_modules and bower components #23484

Merged

sheetalkamat mentioned this pull request May 21, 2018

100% CPU usage and appears hung #24079

Closed

microsoft locked and limited conversation to collaborators Jul 30, 2018

Conversation

sheetalkamat commented Apr 16, 2018

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

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

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants