Use native maps when they're available #12715
Conversation
ghost
force-pushed
the
map5
branch
3 times, most recently
from
69b4ad5
to
62ad1e4
Compare
ghost
assigned 
|
Comparing this branch to Monaconode (v6.9.2, x64)
node (v6.9.2, x86)
tsc (x86)
TFSnode (v6.9.2, x64)
node (v6.9.2, x86)
tsc (x86)
Older node versionsSurprisingly, memory is reduced for node monaco
TFS
|
| export interface Map<T> extends MapLike<T> { | ||
| __mapBrand: any; | ||
| /** It's allowed to get/set into a map with numbers. However, when iterating, you may get strings back due to the shim being an ordinary object (which only allows string keys). */ | ||
| export type MapKey = string | number; |
There was a problem hiding this comment.
We should only allow strings for the keys of Map<T> and go with @ahejlsberg's suggestion to use sparse arrays for "maps" with only numeric keys.
There was a problem hiding this comment.
My test shows a number-keyed Map to be slightly more efficient.
| const map: Map<T> = createObject(null); // tslint:disable-line:no-null-keyword | ||
| const createObject = Object.create; | ||
| /** Create a MapLike with good performance. Prefer this over a literal `{}`. */ | ||
| export function createMapLike<T>(): MapLike<T> { |
There was a problem hiding this comment.
I'd call this createDictionaryObject and keep it internal to the module (don't export it). We only use it in four places across all of src/ and those cases don't look like they'd suffer from just using {}.
There was a problem hiding this comment.
If someone modifies Object.prototype that will cause problems for {}. Isn't that why we have this function?
| const MapCtr = usingNativeMaps ? Map : shimMap(); | ||
|
|
||
| // Keep the class inside a function so it doesn't get compiled if it's not used. | ||
| function shimMap(): { new<T>(): Map<T> } { |
There was a problem hiding this comment.
If you want the Map shim to have support for keys/values/entries iterators, take a look at https://gist.github.com/rbuckton/06d2c79bea46778f9e8bbeca77292087. That way there is less differentiation between what we need out of a native Map and what our shim provides.
| export function someProperties<T>(map: Map<T>, predicate?: (value: T, key: string) => boolean) { | ||
| for (const key in map) { | ||
| if (!predicate || predicate(map[key], key)) return true; | ||
| export const forEachInMap: <T, U>(map: Map<T>, callback: (value: T, key: string) => U | undefined) => U | undefined = usingNativeMaps |
There was a problem hiding this comment.
If we use the MapIterator shim in my gist/comment above, then we don't need to differentiate.
| : <T, U>(map: ShimMap<T>, callback: (value: T, key: string) => U | undefined) => map.forEachInMap(callback); | ||
|
|
||
| /** `forEachInMap` for just keys. */ | ||
| export function forEachKeyInMap<T>(map: Map<{}>, callback: (key: string) => T | undefined): T | undefined { |
There was a problem hiding this comment.
Would it be more efficient to iterate over map.keys() and avoid the closure?
| @@ -262,7 +262,7 @@ namespace Harness.LanguageService { | |||
| this.getModuleResolutionsForFile = (fileName) => { | |||
| const scriptInfo = this.getScriptInfo(fileName); | |||
| const preprocessInfo = ts.preProcessFile(scriptInfo.content, /*readImportFiles*/ true); | |||
| const imports = ts.createMap<string>(); | |||
| const imports = ts.createMapLike<string>(); | |||
There was a problem hiding this comment.
I think we should just use {} for these cases.
| if (response.request_seq in this.callbacks) { | ||
| this.callbacks[response.request_seq](response); | ||
| delete this.callbacks[response.request_seq]; | ||
| const handler = this.callbacks.get(response.request_seq); |
There was a problem hiding this comment.
Either callbacks should be a sparse array, or we should coerce response.request_seq to a string.
| @@ -291,38 +291,30 @@ namespace ts.projectSystem { | |||
| } | |||
|
|
|||
| export class Callbacks { | |||
| private map: { [n: number]: TimeOutCallback } = {}; | |||
| private map = createMap<TimeOutCallback>(); | |||
| this.symbolIdToActionMap[symbolId] = [newAction]; | ||
| const actions = this.symbolIdToActionMap.get(symbolId); | ||
| if (!actions) { | ||
| this.symbolIdToActionMap.set(symbolId, [newAction]); |
| @@ -532,7 +532,7 @@ namespace ts { | |||
| } | |||
|
|
|||
| function getDeclarations(name: string) { | |||
| return result[name] || (result[name] = []); | |||
| return result.get(name) || set(result, name, []); | |||
There was a problem hiding this comment.
Just use multiple lines so we don't need to use set.
…de if necessary.
| @@ -39,6 +39,8 @@ namespace ts { | |||
| return map; | |||
| } | |||
|
|
|||
| export const sparseArray: <T>() => SparseArray<T> = createDictionaryObject; | |||
There was a problem hiding this comment.
What is the purpose of this? This isn't a sparse array.
| } | ||
| } | ||
| return false; | ||
| return someInMap(moduleSymbol.exports, (_, id) => id !== "export="); |
There was a problem hiding this comment.
Why not just use forEachInMap. The someInMap function seems only marginally useful.
| @@ -20070,7 +20097,7 @@ namespace ts { | |||
| // otherwise - check if at least one export is value | |||
| symbolLinks.exportsSomeValue = hasExportAssignment | |||
| ? !!(moduleSymbol.flags & SymbolFlags.Value) | |||
| : forEachProperty(getExportsOfModule(moduleSymbol), isValue); | |||
| : someInMap(getExportsOfModule(moduleSymbol), isValue); | |||
There was a problem hiding this comment.
Why not just use forEachInMap?
| return map; | ||
| } | ||
|
|
||
| export const sparseArray: <T>() => SparseArray<T> = createDictionaryObject; |
There was a problem hiding this comment.
I know you ran some benchmarks regarding the performance of objects vs sparse arrays, but unless the performance difference is fairly significant for our use cases, I'd rather we just continue to leverage actual arrays for sparse arrays. If the difference is significant, then we should use a different name for this function and type.
| } | ||
|
|
||
| /** Whether `predicate` is true for some entry in the map. */ | ||
| export function someInMap<T>(map: Map<T>, predicate: (value: T, key: string) => boolean): boolean { |
There was a problem hiding this comment.
This function seems unnecessary, forEachInMap serves the same purpose.
| } | ||
|
|
||
| /** `someInMap` for just keys. */ | ||
| export function someKeyInMap(map: Map<{}>, predicate: (key: string) => boolean): boolean { |
There was a problem hiding this comment.
This also seems like overkill as we could just use forEachInMap and ignore the first argument.
| } | ||
|
|
||
| /** Copy entries from `source` to `target`. */ | ||
| export function copyMapEntries<T>(source: Map<T>, target: Map<T>): void { |
There was a problem hiding this comment.
nit: Consider a simpler name like copyEntries.
| return values; | ||
| } | ||
|
|
||
| export function multiMapSparseArrayAdd<V>(map: SparseArray<V[]>, key: number, value: V): V[] { |
There was a problem hiding this comment.
This is only used in factory.ts. Why not just use multiMapAdd and cast the key to string?
There was a problem hiding this comment.
This is hard to do since we return the value as a SparseArray, so we'd have to add as any as string at all the external uses too. At least I've moved this function to factory.ts.
| @@ -942,15 +1030,26 @@ namespace ts { | |||
| * Adds the value to an array of values associated with the key, and returns the array. | |||
| * Creates the array if it does not already exist. | |||
| */ | |||
| export function multiMapAdd<V>(map: Map<V[]>, key: string | number, value: V): V[] { | |||
| const values = map[key]; | |||
| export function multiMapAdd<V>(map: Map<V[]>, key: string, value: V): V[] { | |||
There was a problem hiding this comment.
Maybe we should consider adding a createMultiMap<T> that returns a MultiMap<T>:
interface MultiMap<T> extends Map<T[]> {
add(key: string, value: T): T[];
remove(key: string, value: T): void;
}
function createMultiMap<T>() {
const map = <MultiMap<T>>createMap<T[]>();
map.add = multiMapAdd;
map.remove = multiMapRemove;
return map;
}
function multiMapAdd<T>(this: MultiMap<T>, key: string, value: T) { ... }
function multiMapRemove<T>(this: MultiMap<T>, key: string, value: T) { ... }| let result: U; | ||
| for (const key in map) { | ||
| if (result = callback(map[key], key)) break; | ||
| function arrayFrom<T>(iterator: Iterator<T>): T[] { |
There was a problem hiding this comment.
Maybe we should just export this and replace all references to keysOfMap and valuesOfMap.
|
Not a big fan of 2e6f369 as it would make me suspicious of every array in the codebase. |
There was a problem hiding this comment.
This looks good, though we should wait on @ahejlsberg's feedback before merging.
|
@andy-ms I would recommend you update the description of the PR with respect to the final set of new functions added to support using native maps. |
|
@rbuckton Done. And just to be sure, I ran baselines again. Results are still good. Monaconode (v0.12.17, x86)
node (v6.9.2, x64)
node (v6.9.2, x86)
tsc (x86)
TFSnode (v0.12.17, x86)
node (v6.9.2, x64)
node (v6.9.2, x86)
tsc (x86)
|
| } | ||
|
|
||
| /** Create a new map. If a template object is provided, the map will copy entries from it. */ | ||
| export function createMap<T>(template?: MapLike<T>): Map<T> { |
There was a problem hiding this comment.
How often do we actually pass a template to createMap? I seem to remember that functions containing for-in loops are not optimized by V8, so having this optional template stuff may be costly. Perhaps filling a map from a template should just be a separate function.
| target[id] = source[id]; | ||
| if (!source) return; | ||
|
|
||
| source.forEach((sourceSymbol, id) => { |
There was a problem hiding this comment.
Maybe use source && source.forEach(...) and get rid of the if (!source) return above.
| @@ -1,4 +1,4 @@ | |||
| /// <reference path="../factory.ts" /> | |||
| /// <reference path="../factory.ts" /> | |||
There was a problem hiding this comment.
What's up with these ^M line breaks? There are a lot of them in the following files.
There was a problem hiding this comment.
I've looked at this and don't know why it shows a diff here.
- It only shows
^Mon lines that I've changed -- if I had changed line endings in the file, presumably they would show as changed in the whole file. - When I look at the file, it appears to have CRLF line endings (on all lines) on both
masterandmap5. I havecore.autocrlfset tofalse. Tested on both Windows and Linux.
After merge I'll look back to this file to see if it has mixed line endings, but I doubt that.
CC @DanielRosenwasser
Similar to #11354, but simpler.
Map<T>instead ofMap<K, V>, withKassumed to bestring. For number-keyed maps we now use sparse arrays.Set-- this can be done later.sortInV8ObjectInsertionOrder; changes baselines instead. We won't be able to pass all tests when using the shim map, but we no longer run tests on node 0.10 anyway.Added functions
arrayFromIteratorto anArray.arrayFrom(map.keys())replacesObject.keys(map).arrayFrom(map.values())also lets us removereduceProperties, which was only used to compute that.forEachEntry,forEachKeyfor-inloops with early termination,forEachProperty, andsomeProperties.copyEntriescopyProperties.createMultiMapmultiMapAddandmultiMapRemove.mapsAreEqualequalOwnProperties. Used only inreuseProgramStructure.ts, so moved it there.multiMapSparseArrayAddmultiMapAddon what is now a sparse array. Used only infactory.ts, so moved it there.mapEntriesmapObject.We also get rid of
isEmptyand replace withmap.size === 0.