-
Notifications
You must be signed in to change notification settings - Fork 684
perf(tspath): optimize hasRelativePathSegment
#1569
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 hasRelativePathSegment
#1569
Conversation
│ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ HasRelativePathSegment/foo/bar/baz-12 29.10n ± 2% 14.79n ± 1% -49.16% (p=0.000 n=10) HasRelativePathSegment/./some/path-12 2.065n ± 0% 2.039n ± 2% -1.26% (p=0.022 n=10) HasRelativePathSegment//foo/./bar/../../.-12 7.030n ± 0% 2.025n ± 0% -71.20% (p=0.000 n=10) HasRelativePathSegment/foo/foo/foo/foo/foo/...etc-12 9.076n ± 1% 2.308n ± 1% -74.56% (p=0.000 n=10) geomean 7.869n 3.446n -56.21%
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.
Ran with go test -fuzz=FuzzToFileNameLowerCase ./internal/tspath
for a while and nothing broke.
Thanks for this; when I improved this the first time, it was way better than the original one, but I always meant to come back and improve it further. It's just never been a bottleneck for anything we see in tsc/LSP.
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.
Oops, I ran the wrong one. This PR is FuzzHasRelativePathSegment
.
go test -fuzz=FuzzHasRelativePathSegment ./internal/tspath
immediately fails with:
fuzz: elapsed: 0s, gathering baseline coverage: 0/9 completed
fuzz: elapsed: 0s, gathering baseline coverage: 9/9 completed, now fuzzing with 20 workers
fuzz: minimizing 37-byte failing input file
fuzz: elapsed: 0s, minimizing
--- FAIL: FuzzHasRelativePathSegment (0.11s)
--- FAIL: FuzzHasRelativePathSegment (0.00s)
path_test.go:616: assertion failed: false (bool) != true (bool)
Failing input written to testdata/fuzz/FuzzHasRelativePathSegment/9cd9b70960b4a733
To re-run:
go test -run=FuzzHasRelativePathSegment/9cd9b70960b4a733
FAIL
exit status 1
FAIL github.com/microsoft/typescript-go/internal/tspath 0.129s
Which tests the string /
.
🫣 oops, thanks for testing, should be fixed now:
|
This pull request optimises
hasRelativePathSegment
, delivering a ~56% perf improvement.This function is Called
27_628_418
times via tsgolint when linting vscode repo.this function has tests here to ensure we don't regress vs the old version:
typescript-go/internal/tspath/path_test.go
Lines 610 to 618 in d6c0867