-
Notifications
You must be signed in to change notification settings - Fork 12.9k
[experiment] Naively make normalizeSlashes a no-op outside of Windows #53173
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
Conversation
@typescript-bot perf test this |
Heya @jakebailey, I've started to run the perf test suite on this PR at 5597a9a. You can monitor the build here. Update: The results are in! |
@typescript-bot perf test this |
Heya @jakebailey, I've started to run the perf test suite on this PR at 8973242. You can monitor the build here. Update: The results are in! |
@typescript-bot test this |
Heya @jakebailey, I've started to run the extended test suite on this PR at 8973242. You can monitor the build here. |
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 8973242. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 8973242. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:
CompilerComparison Report - main..53173
System
Hosts
Scenarios
TSServerComparison Report - main..53173
System
Hosts
Scenarios
StartupComparison Report - main..53173
System
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
@jakebailey Here they are:
CompilerComparison Report - main..53173
System
Hosts
Scenarios
TSServerComparison Report - main..53173
System
Hosts
Scenarios
StartupComparison Report - main..53173
System
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
@typescript-bot test this |
Heya @jakebailey, I've started to run the extended test suite on this PR at 8973242. You can monitor the build here. |
Potentially less noisy DetailsLoading benchmark and comparing to baseline.
System
Hosts
Scenarios
It's maybe 1-2 percent at parse time? Given the regular runner sees no change, maybe there's no benefit to this anymore now that this function is optimized via #44100 and #50154? |
@typescript-bot test this |
Heya @jakebailey, I've started to run the extended test suite on this PR at 8973242. You can monitor the build here. |
@typescript-bot test this |
Heya @jakebailey, I've started to run the extended test suite on this PR at 8973242. You can monitor the build here. |
@typescript-bot test this |
Heya @jakebailey, I've started to run the extended test suite on this PR at 8973242. You can monitor the build here. |
@typescript-bot test this |
Heya @jakebailey, I've started to run the extended test suite on this PR at 8973242. You can monitor the build here. |
What OS does the perf suite run on? |
OS is Linux, a recent version of Ubuntu, which is the OS we'd want to test for this kind of change. The implausible perf change is from those projects using the wrong kinds of slashes, e.g. monaco has a file like this: /// <reference path="lib.d.ts" />
/// <reference path="client\extlib.d.ts" />
/// <reference path="client\vs\base\arrays.ts" />
/// <reference path="client\vs\base\assert.ts" />
/// <reference path="client\vs\base\async.ts" />
/// <reference path="client\vs\base\cookiesUtil.ts" />
/// <reference path="client\vs\base\diagnostics.ts" />
/// <reference path="client\vs\base\env.ts" />
// ... So it's not actually type checking these files because it didn't find them. And, this is also the sort of thing we probably don't want to be breaking. |
I'm a bit surprised that works at all. For example, I'd expect |
I'm not super surprised given those comments are effectively XML which has different escape rules. But, I also doubt anyone is using these slashes in real codebases these days either. |
For #44174.