Switch to ES Map/Set internally #33771
Conversation
rbuckton
force-pushed
the
esMapSetShim
branch
3 times, most recently
from
e4769a2 to
374ce8b
Compare
rbuckton
changed the title
rbuckton
changed the title
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 79eb8ee. You can monitor the build here. Update: The results are in! |
|
@rbuckton Here they are:Comparison Report - master..33771
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# Conflicts: # src/services/refactors/extractSymbol.ts
| @@ -3204,7 +3204,7 @@ namespace ts { | |||
| // If the emit is enabled make sure that every output file is unique and not overwriting any of the input files | |||
| if (!options.noEmit && !options.suppressOutputPathCheck) { | |||
| const emitHost = getEmitHost(); | |||
| const emitFilesSeen = createMap<true>(); | |||
| const emitFilesSeen = new Set<string>(); | |||
There was a problem hiding this comment.
This breaks verifyEmitFilePath unless we cast the key to Path.
There was a problem hiding this comment.
emitFilesSeen.add(emitFileKey); is path in there though.
|
@sheetalkamat: I've made most of the suggested key changes, but there are some that would result in type errors that I do not have enough context to resolve. |
|
While there are still some outstanding cases to investigate regarding keys, I don't know that they should be a blocker for this PR. Without this PR those keys are currently still typed as |
# Conflicts: # src/server/project.ts # src/services/types.ts
|
@sheetalkamat can you take another look? |
There was a problem hiding this comment.
Apart from two suggestions things look good.
Here are two questions i still have:
1: There are some places where you replaced createMap() with new Map() but not all. Was there a reason for that
2. Do we really care about performance of shim implementaion if we were really using the native maps before.. Why not use .toString() on key for shim to keep it simple and compact if its going to be used rarely.
|
Sorry didnt notice your questions on the previous comments,going through them now |
Those were mostly ad-hoc. I am planning on putting together a follow-up PR that replaces all other references and removes
Using If the recommendation is to ignore performance for the shim and simplify the design, then I'd likely just do away with the buckets and use the linked list implementation used for One of the stated goals for this PR is to allow us to have non-primitive keys. While I am not making heavy use of it in this PR, I plan to migrate some of our maps/sets to accept |
# Conflicts: # src/compiler/transformers/utilities.ts
|
@sheetalkamat: I've put together a simpler (though less performant) version of the shim that just uses a linked list. Should I use that one instead? |
|
I'll go ahead and switch to the simpler |
|
@sheetalkamat: Would you care to take one more look over the shim implementation? I repurposed the existing doubly-linked list that was added to preserve iteration order, which should result in less memory usage overall. |
|
@sheetalkamat: I've filed #39264 which leverages the work in this PR and comprehensively removes calls to |
* upstream/master: LEGO: check in for master to temporary branch. Preserve newlines between try/catch/finally, if/else, do/while (microsoft#39280) not narrow static property without type annotation in constructor. (microsoft#39252) Switch to ES Map/Set internally (microsoft#33771) fix(38840): omit completions for a spread like argument in a function definition (microsoft#38897) fix(38785): include in NavigationBar child items from default exported functions (microsoft#38915) LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. Avoid effect of element access expression (microsoft#39174) Update typescript-eslint to 3.4.1-alpha.1 (microsoft#39260) Handle 'keyof' for generic tuple types (microsoft#39218) Disable unsound T[K] rule in subtype relations (microsoft#39249) LEGO: check in for master to temporary branch. Upgrade typescript-eslint version (microsoft#39242) Handle recursive type references up to a certain level of expansion in inference (microsoft#38011) Do not consider binding patterns in contextual types for return type inference where all the signature type parameters have defaults (microsoft#39081) LEGO: check in for master to temporary branch. # Conflicts: # src/compiler/program.ts # src/compiler/types.ts
Currently leaving this as a Draft PR for discussion. This improves our
Mapshim to support non-string keys, and adds a shim forSetso that we might consider using it in the compiler as well.These shims exist for a vanishingly rare corner case of TypeScript running in an environment without native implementations (at this point, primarily just IE).