-
Notifications
You must be signed in to change notification settings - Fork 720
perf(tspath): optimize the fast path case for ToFileNameLowerCase
#1558
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(tspath): optimize the fast path case for ToFileNameLowerCase
#1558
Conversation
Can you include some benchmarks that show the difference? As in, Go benchmarks in I'm not surprised by this PR, of course; |
mind clarifying exactly what your looking for? typescript-go/internal/tspath/path_test.go Line 545 in 4be54c5
exists already, which is what i've been using to bench the perf differences. are you looking for a bench function that compares the two (old vs new impl) |
@microsoft-github-policy-service agree |
1 similar comment
@microsoft-github-policy-service agree |
Ah, yes. I didn't realize that's what you were doing. I think I was expecting results through https://pkg.go.dev/golang.org/x/perf/benchstat. Comparing main and this PR with
Which I think matches what you were quoting. Surprised the other cases get slower, honestly. |
yess, although I can write go, it's been a while and I am far from an expert, i'm also not up to date with the latest tooling, so there's definitly cases where I could be using a tool to do what i'm doing manually 😅
the other cases will have slowed down because we now have to iterate over the string twice, this shouldn't be a problem in 99% of cases since most file paths will be ascii, and won't contain that special |
Or at least for usernames with non-ASCII characters, they'll be early in the path via |
For the record, I do: $ go test -run=- -bench='BenchmarkToFileNameLowerCase' -benchmem -count=10 ./internal/tspath | tee old.txt
# switch branch
$ go test -run=- -bench='BenchmarkToFileNameLowerCase' -benchmem -count=10 ./internal/tspath | tee new.txt
$ benchstat old.txt new.txt |
Just out of curiosity, where did you experience this being a bottleneck? |
We are diving into i.e. running tsgolint in large monorepos (1000+ files). |
Yep to add to Boshen's point, i'm testing this on vscode repo (~6100) files, during which, this function is called 10 million + time. thanks for the extra bechmarking info Jake, i'll be using that next time! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fuzz test is clean after running for a bit.
(We don't check these in CI, it would be good if we did though they take a while and you need to select a specific test to fuzz, not just ask for all to be fuzzed.)
When using project services,
ToFileNameLowerCase
is called 10, 312, 056 times on various paths, this means that it's incredibly hot path.Since most projects won't hav e non-ascii filenames, we can optimize for this case. This yields up to a 5x perf improvement in some scenarios.
Please feel free to close, if the perf degregation againt non-ascii file paths outweighs the perf gains for the common case