Use more explicit operations in core helpers (and other nits) #58873
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot perf test this |
|
@DanielRosenwasser Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot perf test this faster predictable [email protected] |
|
Starting jobs; this comment will be updated as builds start and complete.
|
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
jakebailey
reviewed
Jun 17, 2024
jakebailey
reviewed
Jun 17, 2024
src/compiler/core.ts
Outdated
Comment on lines
706
to
685
| /** @internal */ | ||
| /** @internal * |
There was a problem hiding this comment.
I dunno how that happened, but it is pretty funny that this is never used.
DanielRosenwasser
changed the title
|
@typescript-bot perf test this |
jakebailey
reviewed
Jun 17, 2024
Comment on lines
+1755
to
+1756
| // NOTE: arrayFrom means that if the callback mutates the underlying collection, | ||
| // we won't have an accurate set of values |
There was a problem hiding this comment.
Suggested change
| // NOTE: arrayFrom means that if the callback mutates the underlying collection, | |
| // we won't have an accurate set of values | |
| // NOTE: arrayFrom prevents `action` from modifying the map while we iterate |
There was a problem hiding this comment.
To be honest, I'm not sure if the copy was intentional.
There was a problem hiding this comment.
Nothing fails when it's removed, huh
There was a problem hiding this comment.
Neither behavior is really easy to make guarantees about without extra book-keeping.
Right now if you add a new value and it's added into an array under the hood, you will witness a new value if you're in the middle of iterating, but not if you've already finished iterating over all values with a given key.
On the other hand, if the multimap stores a single value, you'll never see any added values as you iterate.
jakebailey
reviewed
Jun 17, 2024
jakebailey
reviewed
Jun 17, 2024
|
@DanielRosenwasser Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @@ -2145,6 +2120,14 @@ export function compareTextSpans(a: Partial<TextSpan> | undefined, b: Partial<Te | |||
| return compareValues(a?.start, b?.start) || compareValues(a?.length, b?.length); | |||
| } | |||
|
|
|||
| /** @internal */ | |||
| export function maxBy<T>(arr: readonly T[], init: number, mapper: (x: T) => number): number { | |||
There was a problem hiding this comment.
I guess this could always be a reduceMapped instead.
jakebailey
approved these changes
Jun 18, 2024
This was referenced Jun 18, 2024
| export function arraysEqual<T>(a: readonly T[], b: readonly T[], equalityComparer: EqualityComparer<T> = equateValues): boolean { | ||
| return a.length === b.length && a.every((x, i) => equalityComparer(x, b[i])); | ||
| } | ||
|
|
||
| /** @internal */ | ||
| export function indexOfAnyCharCode(text: string, charCodes: readonly number[], start?: number): number { |
There was a problem hiding this comment.
If this function is ever called in a hot path it would probably be more efficient to convert charCodes to a Set first to reduce the O(text*codes) to O(text)
| if (!some(array2)) return array1; | ||
| if (!some(array1)) return array2; | ||
| if (array2 === undefined || array2.length === 0) return array1; | ||
| if (array1 === undefined || array1.length === 0) return array2; | ||
| return [...array1, ...array2]; |
There was a problem hiding this comment.
if indexed array access is significantly faster via avoiding iterator semantics, using array1.concat(array2) is probably faster for the same reason.
Comment on lines
+881
to
886
| if (result ?? !v) { | ||
| result ??= array.slice(0, i); | ||
| if (v) { | ||
| result.push(v); | ||
| } | ||
| } |
There was a problem hiding this comment.
This might be more readable as:
Suggested change
| if (result ?? !v) { | |
| result ??= array.slice(0, i); | |
| if (v) { | |
| result.push(v); | |
| } | |
| } | |
| if (v) { | |
| // If result has been initialized it is looking to collect truthy values separately. | |
| // Otherwise it will be initialized when we hit our first falsy value. | |
| result?.push(v); | |
| } else { | |
| // If this is our first falsy value, copy over the current stretch of truthy values. | |
| result ??= array.slice(0, i); | |
| } |
Comment on lines
1684
to
1685
| if (!multiMap.has(hash)) return false; | ||
| const candidates = multiMap.get(hash)!; |
There was a problem hiding this comment.
Does this utility function expressly support undefined being a meaningful value in the set? Otherwise this would be more efficient to call get and do a check against undefined instead of calling into the map twice.
| @@ -305,7 +306,7 @@ function createTabularErrorsDisplay(filesInError: (ReportFileInError | undefined | |||
|
|
|||
| const numberLength = (num: number) => Math.log(num) * Math.LOG10E + 1; | |||
| const fileToErrorCount = distinctFiles.map(file => ([file, countWhere(filesInError, fileInError => fileInError!.fileName === file!.fileName)] as const)); | |||
| const maxErrors = fileToErrorCount.reduce((acc, value) => Math.max(acc, value[1] || 0), 0); | |||
| const maxErrors = maxBy(fileToErrorCount, 0, value => value[1]); | |||
|
|
|||
| const headerRow = Diagnostics.Errors_Files.message; | |||
| const leftColumnHeadingLength = headerRow.split(" ")[0].length; | |||
There was a problem hiding this comment.
Suggested change
| const leftColumnHeadingLength = headerRow.split(" ")[0].length; | |
| const leftColumnFirstSpace = headerRow.indexOf(" "); | |
| const leftColumnHeadingLength = leftColumnFirstSpace < 0 ? headerRow.length : leftColumnFirstSpace; |
Though I assume this line isn't a hot path.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR takes care of a few nits in the codebase, including:
??over||when possible.forloops incore.tsfor all iteration over arrays.undefinedoncore.tshelpers.Many of these are rooted in the need for core utilities to be faster, and therefore use fewer implicit operations like iterator usage, truthiness, etc.
There are some other things I noticed, but this seemed like a reasonable first pass.