Log top 5 largest files when TS language service is disabling.#19315
Conversation
|
If you find sorting and managing the top5 list is too time-consuming, maybe we can just report the largest file. |
This second pass seemingly should be avoid. Bug there.
|
I notice this method is being called twice on the same project. So it seems that it double counted the file size. |
| this.projectToSizeMap.forEach(val => (availableSpace -= (val || 0))); | ||
|
|
||
| let totalNonTsFileSize = 0; | ||
| let top5LargestFiles = [] as { name: string, size: number }[]; |
There was a problem hiding this comment.
Use an annotation, not a type assertion.
|
#19326 is probably the right issue to track this. |
|
I think that in the error case, you should just sort the list of all files and slice off the top 5 instead of inserting the logic into the loop. |
Sure, can do that too. But in order to do that, I have to use |
|
Any comment on the log message? |
|
In the last commit, I use Error handling and logging code should not obstruct main code path. An alternative is to make it a pure function. |
| } | ||
|
|
||
| const size = this.host.getFileSize(name); | ||
| totalNonTsFileSize += size; |
There was a problem hiding this comment.
do you need to compute this again?
There was a problem hiding this comment.
consider:
fileNames
.map(f => hasTypeScriptFileExtension(name) ? 0 : { name, size: host.getFileSize(name) })
.sort((a, b) => b.size - a.size)
.slice(0,5)
.map(f => `${f.name} (${f.size})`)
.join(",")There was a problem hiding this comment.
That's a good idea, but I'll separate formatting from the chain.
| totalNonTsFileSize += this.host.getFileSize(fileName); | ||
|
|
||
| if (totalNonTsFileSize > maxProgramSizeForNonTsFiles) { | ||
| logExceedLimit.call(this); |
There was a problem hiding this comment.
do we need top 5? or can we jsut log the largest so far? if so, we can keep the largest file in a local.
There was a problem hiding this comment.
W can do that too. I initially do top 5 just to get more information so I can exclude/fix multiple files at the same time.
I change to passing that value in. However, it is not an accurate number because in the main loop it exits once |
| this.projectToSizeMap.set(name, totalNonTsFileSize); | ||
| return false; | ||
|
|
||
| function logExceedLimit(this: ProjectService, totalNonTsFileSize: number) { |
There was a problem hiding this comment.
why do you need the this here?
There was a problem hiding this comment.
just pass in the logger/host, or make the function return a string.
There was a problem hiding this comment.
Sure, that would be even better.
This PR mitigates an issue on vscode: microsoft/vscode#35549
It will now log the top 5 largest files loaded.
Suggestion on how to format the message is welcome.
There is no test added 😞 because I can't find a way to test this private method, and also stubbing the logger to validate the output.