perf: rework snapshot hot paths

[43081j]

fromNonSveltePath

This function was originally using replace to unix-ify paths before
then testing against them with endsWith.

We can instead use static patterns on the unaltered path regardless of
separator.

deleteUnresolvedResolutionsFromCache

This was originally using split to "parse" the cache key into a
file/specifier pair.

We don't actually need to do this as we know there's always one
occurrence of CACHE_KEY_SEPARATOR. So we can instead just slice on
the index of the separator.

Additionally, we only compute any of this the first time we encounter an
unresolved cache entry.

Possible bug

I then noticed that fileNameWithoutEnding could be a (from
a.svelte), for example. This then meant we would delete all cache
entries with specifiers containing a (since we did
specifier.includes(fileNameWithoutEnding)).

I have changed this to explicitly test for these two cases instead:

• Exact match (i.e. the specifier is exactly fileNameWithoutEnding)
• Path-like string includes specifier (/{fileNameWithoutEnding}.)

For example, a.svelte would become a. We then want to check for
/a. to be able to match /a.svelte, /a.svelte.js, etc. Nobody can
import it without a / preceding it since then it'd be a bare
specifier.

getLastPartOfPath

This now uses slice on the last separator rather than splitting only
to retrieve the last part.

diagnostics

In the root svelte-check logic, we were previously allocating an array of the diagnostics from various sources then later discarding that array (garbage collecting it).

I have changed this to instead iterate through each diagnostics set one by one, so we never allocate a new array.

Why, James?

Locally this reduces svelte-check in shadcn-svelte's own docs from ~14s to ~10s.

Most of this is the change in keying strategy (getKey), but the lazy name computation helped a lot too.

[changeset-bot]

🦋 Changeset detected

Latest commit: 6558687

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
svelte-language-server	Patch
svelte-check	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[jasonlyu123]

It shouldn't be possible for two sibling files to resolve the same specifier
to two different modules.

It's possible when one file is a .cts and the other is a .mts. In a "native" Node.js ESM module, you can't omit the file extension or omit the "index.js". So the resolution result will be different. As for the "per directory cache", we have a tsModuleCache variable that should cache per directory module resolution results. So this optimisation shouldn't be necessary. Did you also see an improvement here?

For the possible bug with "fileNameWithoutEnding", it's more of an "overly aggressive" cache invalidation. We might have to think more about if there are other cases where the new check doesn't cover.

[43081j]

hmm, you are right.

there must be something we can do here though. the current keying means a large project repeatedly resolves the same module many times. this usually involves file system calls too, which is what takes so long (since ts' own resolveModule hits the file system for path resolution).

i didn't encounter a directory cache, can you point me at the code you're talking about?

For the possible bug with "fileNameWithoutEnding", it's more of an "overly aggressive" cache invalidation. We might have to think more about if there are other cases where the new check doesn't cover

extremely aggressive cache invalidation 😅

quite a lot of files will overmatch. what if i have process.js - whoops we just invalidated process (bare specifier) too.

short filenames like a, css, util, etc cause this cache to be near worthless. so yes, we really should do something about that

[jasonlyu123]

The tsModuleCache variable. We completely clear the per-directory cache in deleteUnresolvedResolutionsFromCache because per-directory clear is an internal API. Maybe when you're debugging, it has already been cleared. However, even after it's cleared, there is still per-file cache, so it won't resolve everything again.

Ideally, this shouldn't trigger during svelte-check if you're not in watch mode. So I am not really sure what the problem is you're encountering. Can you open an issue with more details on the issue and a reproduction repository?
Eh. Never mind, it does clear it. I think you can find a way to eliminate the deleteUnresolvedResolutionsFromCache call from startup. The new files found in the module resolution also shouldn't trigger deleteUnresolvedResolutionsFromCache.

[43081j]

We completely clear the per-directory cache in deleteUnresolvedResolutionsFromCache

do we?

deleteUnresolvedResolutionsFromCache clears the cache for a specific file regardless of basename. it doesn't clear the cache for a directory

given the current logic before this PR:

language-tools/packages/language-server/src/plugins/typescript/module-loader.ts

Lines 69 to 73 in dec37ea

	const [containingFile, moduleName = ''] = key.split(CACHE_KEY_SEPARATOR);
	if (moduleName.includes(fileNameWithoutEnding)) {
	this.cache.delete(key);
	this.pendingInvalidations.add(containingFile);
	}

a key might be /code/foo.js:::./bar.js

if we're deleting caches for /code/bar.js, we will look up all keys which contain bar and remove them.

for example, if we had this cache:

/code/foo.js::./bar.js
/code/foo.js::./baz.js
/code/utils/util0.js::../bar.js

we will clear all the bar resolutions so we have only:

/code/foo.js::./baz.js

we don't clear the per directory cache

edit: ok cool you realised this 😂

[43081j]

FYI im not encountering any issue, and don't have any obvious reproduction

i decided to profile svelte-check to see if we can speed it up and here's the results

if you want more info you're probably better off chatting to me on discord about it or something as this is the result of reading through a whole bunch of CPU profiles

using shadcn-svelte as a test subject

[jasonlyu123]

You're saying it improves the performance of svelte-check and not the watch mode, right? Then I think it's better to make it not call deleteUnresolvedResolutionsFromCache in the first place. The optimisation you make should still be useful in the editor and svelte-check --watch. So we can still keep the change, just not "change key to the directory name" part.

[43081j]

yes i'm solely profiling svelte-check right now

if we can avoid calling it altogether, great 😄

i can revert the key change for now. can you explain a bit more where you think we can avoid calling the deletion function, and why?

[dummdidumm]

Re key: Could we only use the filename if we detect the file name ending is .cts/.mts/.cjs/.mjs? That way we would store the potentially-resolved-differently modules in a separate key, and given that those file endings are very rare it's unlikely it has any impact of perf.

[43081j]

FYI i have discarded the key change for now and got rid of a bit of garbage collection around diagnostic iteration

we could possibly merge this one as-is when we're happy and work on the key stuff in a follow up

i would like to figure it out since it seems we invalidate caches far too often right now

doing something with extensions makes sense to me

Edit:

I'll fix the tests when I'm next at a laptop 👍

[43081j]

ok ive reverted the filePart includes check so this branch should now pass tests (it does locally)

we should land #2853 first and ill sort conflicts

[43081j]

i've rebased @dummdidumm and this now only contains the additional smaller optimisations (they each still save us garbage collection though so worth having)

[dummdidumm]

thank you for starting this work and nudging us towards these perf improvements!

[43081j]

no worries!

i just pushed a quick prettier change since it wanted single quotes in the changeset