Uh oh!
There was an error while loading. Please reload this page.
Add StringMap, NumberMap, and StringSet data structures - #10230
Add StringMap, NumberMap, and StringSet data structures#10230Deleted user (ghost) wants to merge 6 commits into
Conversation
…e available, to replace the old Map, which is now ObjMap.
| } | ||
| /** String-keyed Map. */ | ||
| export interface SMap<V> extends MapCommon<string, V> { |
There was a problem hiding this comment.
nit: I think it will be easier to read if you just use StringMap instead of SMap and same as NumberMap for NMap
There was a problem hiding this comment.
On that topic, what should we name ObjMap? (The keys aren't objects. The map itself is an object, which is used like a StringMap.)
There was a problem hiding this comment.
NumberKeyedMap, StringKeyedMap, ObjectBackedMap. With good autcomplete, there's no real reason for confusing or ambiguous hungarianesque notation.
| const typeLiteralSymbol = createSymbol(SymbolFlags.TypeLiteral, "__type"); | ||
| addDeclarationToSymbol(typeLiteralSymbol, node, SymbolFlags.TypeLiteral); | ||
| typeLiteralSymbol.members = { [symbol.name]: symbol }; | ||
| typeLiteralSymbol.members = singletonMap(symbol.name, symbol); |
There was a problem hiding this comment.
What's the purpose of singletonMap?
There was a problem hiding this comment.
Thought it was neater than new SMap([[symbol.name, symbol]]).
There was a problem hiding this comment.
Nothing about this is like a singleton in the design pattern sense (it's a loaded term), so the name is a bit confusing. createStringKeyedMap would be more straightforward, if you must wrap the constructor.
Wesley Wigham (weswigham)
commented
Aug 9, 2016
This appears to sort object type keys (which is causing what should be a transparent change to manifest in our baselines). With this change sort order is now insertion order (which arguably is nice), however this is likely a problem because the polyfills right now don't do the same sorting. (This may surface as node 0.10 failing CI with mismatching baselines once the other bugs are fixed) |
| export const SSet: SSetConstructor = Set ? Set : ShimSSet; | ||
| /** False iff there are any values in the set. */ | ||
| export function setIsEmpty(set: SSet): boolean { |
There was a problem hiding this comment.
Should this be isSetEmpty?
Raghav Katyal (rakatyal)
commented
Aug 9, 2016
Could you also please share some numbers on how this effect both memory and performance in the typescript repo and other use cases? |
Deleted user (ghost)
commented
Aug 10, 2016
Overall, we get about a 10% reduction of both memory and total time. (We could get even more by changing the emitter to use StringMaps.) (Note: since file system accesses are cached, I throw away the first run, which has a large I/O read time.) VSCodeTesting on the latest checkout of Microsoft/vscode: Constant:
Typescript servicesTesting on the
Single fileJust a file with
Interactive testWhat I did:
|
Mohamed Hegazy (mhegazy)
commented
Sep 13, 2016
Andy (Andrewkraft) (@Andy-MS) what were the numbers if we are using the shims and not the native implementation? |
The shims caused an increase in memory due to indirection: the shim was an object pointing to another object in dictionary mode. With native maps: With the shims always used: I have created a new PR #11210 which fixes this issue. |
This switches us to using native Maps and Sets where possible.
Since we can't guarantee that the native
Mapwill exist, we instead haveStringMapandNumberMapfor string-keyed and number-keyed maps, with shims using objects and arrays.Many remaining uses of the old
Mapmay be better off asStringMap, but I've left alone anything that looks public-facing.I've also mostly left
declarationEmitterandemitteralone. When we merge the transforms branch in we should use the new maps there.I'd like to assign Raghav Katyal (@rakatyal) to this but it looks like he's not yet on the available assignees list.