Use native maps when they're available #11354
Merged
Conversation
This file contains 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
|
Here is a performance test for doing the same map lookup once vs twice. const Benchmark = require("benchmark")
const crypto = require("crypto")
const N = 10000
const words = []
for (let i = 0; i < N; i++)
words[i] = crypto.randomBytes(8).toString('hex')
function createMap() {
const m = new Map()
for (let i = 0; i < N; i += 2)
m.set(words[i], i)
return m
}
function createObject() {
const o = Object.create(null)
o["__"] = undefined
delete o["__"]
for (let i = 0; i < N; i += 2)
o[words[i]] = i
return o
}
let correctSum = 0
for (let i = 0; i < N; i += 2)
correctSum += i
const suite = new Benchmark.Suite
suite.add("map has-then-get", () => {
const map = createMap()
let sum = 0
for (let i = 0; i < N; i++)
if (map.has(words[i]))
sum += map.get(words[i])
if (sum !== correctSum) throw new Error(sum)
return sum
})
suite.add("map just get", () => {
const map = createMap()
let sum = 0
for (let i = 0; i < N; i++) {
const x = map.get(words[i])
if (x !== undefined)
sum += x
}
if (sum !== correctSum) throw new Error(sum)
return sum
})
suite.add("object has-then-get", () => {
const o = createObject()
let sum = 0
for (let i = 0; i < N; i++)
if (words[i] in o)
sum += o[words[i]]
if (sum !== correctSum) throw new Error(sum)
return sum
})
suite.add("object just get", () => {
const o = createObject()
let sum = 0
for (let i = 0; i < N; i++) {
const x = o[words[i]]
if (x !== undefined)
sum += x
}
if (sum !== correctSum) throw new Error(sum)
return sum
})
suite.add("map double get", () => {
const map = createMap()
let sum = 0
for (let i = 0; i < N; i += 2) {
sum += map.get(words[i])
sum += map.get(words[i])
}
if (sum !== correctSum * 2) throw new Error(sum)
return sum
})
suite.add("map single get", () => {
const map = createMap()
let sum = 0
for (let i = 0; i < N; i += 2) {
const got = map.get(words[i])
sum += got
sum += got
}
if (sum !== correctSum * 2) throw new Error(sum)
return sum
})
suite.add("object double get", () => {
const o = createObject()
let sum = 0
for (let i = 0; i < N; i += 2) {
sum += o[words[i]]
sum += o[words[i]]
}
if (sum !== correctSum * 2) throw new Error()
return sum
})
suite.add("object single get", () => {
const o = createObject()
let sum = 0
for (let i = 0; i < N; i += 2) {
const got = o[words[i]]
sum += got
sum += got
}
if (sum !== correctSum * 2) throw new Error(sum)
return sum
})
suite.on("complete", function() {
this.forEach(({name, stats}) => {
console.log(`${name}: ${stats.mean * 1000}ms`)
})
})
suite.on("error", err => {
throw err.target.error
})
suite.run()Looks like removing uses of |
ghost
mentioned this pull request
ghost
force-pushed
the
map4
branch
2 times, most recently
from
1f080a8
to
6fc4ead
Compare
|
This would be a breaking change. We have publicly exported maps because we have |
rbuckton
reviewed
Oct 6, 2016
| @@ -57,6 +57,7 @@ function measure(marker) { | |||
| } | |||
|
|
|||
| var compilerSources = [ | |||
| "dataStructures.ts", | |||
There was a problem hiding this comment.
I'd be tempted to call this collections.ts, since we already have a types.ts and this is more specific.
There was a problem hiding this comment.
What do you think of moving our array utilities to collections.ts?
rbuckton
reviewed
Oct 6, 2016
| const fullyFeaturedMaps = usingNativeMaps && "keys" in Map.prototype && "values" in Map.prototype && "entries" in Map.prototype; | ||
|
|
||
| /** Extra Map methods that may not be available, so we must provide fallbacks. */ | ||
| interface FullyFeaturedMap<K, V> extends Map<K, V> { |
ghost
mentioned this pull request
mhegazy
suggested changes
Oct 11, 2016
| * In runtimes without Maps, this is implemented using a sparse array. | ||
| * This is generic over the key type because it is usually an enum. | ||
| */ | ||
| export const NumberMap: NumberMapConstructor = usingNativeMaps ? Map : class ShimNumberMap<K extends number, V> implements Map<K, V> { |
There was a problem hiding this comment.
consider merging the shims for NumberMap and StringMap.
| * In runtimes without Maps, this is implemented using a sparse array. | ||
| * This is generic over the key type because it is usually an enum. | ||
| */ | ||
| export const NumberMap: NumberMapConstructor = usingNativeMaps ? Map : class ShimNumberMap<K extends number, V> implements Map<K, V> { |
There was a problem hiding this comment.
for stylistic consistency do not use classes.
There was a problem hiding this comment.
can you share the numbers after removing classes?
There was a problem hiding this comment.
It ends up taking 20% longer. 🤕
Here are the measurements for running map4 with closure shims, with "Delta" relative to map4 with class shims.
Monaco - node (v6.7.0, x86)
| Project | Baseline | Current | Delta | Best | Worst |
|---|---|---|---|---|---|
| Memory used | 203,967k (± 0.01%) | 224,241k (± 0.01%) | +20,274k (+ 9.94%) | 224,220k | 224,263k |
| Parse Time | 2.27s (± 0.39%) | 2.39s (± 3.65%) | +0.12s (+ 5.30%) | 2.32s | 2.51s |
| Bind Time | 0.68s (± 1.17%) | 0.83s (± 1.52%) | +0.15s (+ 21.41%) | 0.81s | 0.84s |
| Check Time | 3.86s (± 2.12%) | 4.72s (± 3.44%) | +0.86s (+ 22.15%) | 4.55s | 4.89s |
| Emit Time | 1.62s (± 2.07%) | 2.30s (± 14.05%) | +0.68s (+ 41.80%) | 2.04s | 2.72s |
| Total Time | 8.43s (± 1.26%) | 10.23s (± 3.36%) | +1.80s (+ 21.34%) | 9.78s | 10.47s |
TFS - node (v6.7.0, x86)
| Project | Baseline | Current | Delta | Best | Worst |
|---|---|---|---|---|---|
| Memory used | 175,828k (± 0.01%) | 188,541k (± 0.01%) | +12,713k (+ 7.23%) | 188,529k | 188,551k |
| Parse Time | 1.40s (± 0.75%) | 1.51s (± 4.24%) | +0.11s (+ 7.80%) | 1.46s | 1.60s |
| Bind Time | 0.57s (± 1.02%) | 0.68s (± 2.26%) | +0.12s (+ 20.63%) | 0.67s | 0.70s |
| Check Time | 3.00s (± 0.53%) | 3.60s (± 3.55%) | +0.60s (+ 19.88%) | 3.41s | 3.69s |
| Emit Time | 1.37s (± 1.09%) | 1.71s (± 5.74%) | +0.34s (+ 25.18%) | 1.66s | 1.86s |
| Total Time | 6.33s (± 0.28%) | 7.50s (± 2.73%) | +1.16s (+ 18.38%) | 7.24s | 7.71s |
Here's the code I tested with (with equivalents for NumberMap and StringSet):
export const createStringMap: <T>(pairs?: [string, T][]) => Map<string, T> = usingNativeMaps
? <T>(pairs?: [string, T][]) => new Map(pairs)
: <T>(pairs?: [string, T][]) => {
let data = createDictionaryModeObject<T>();
if (pairs) {
for (const [key, value] of pairs) {
data[key] = value;
}
}
return {
clear(): void {
data = createDictionaryModeObject<T>();
},
delete(key: string): void {
delete data[key];
},
get(key: string): T {
return data[key];
},
has(key: string): boolean {
// tslint:disable-next-line:no-in-operator
return key in data;
},
set(key: string, value: T): void {
data[key] = value;
},
forEach(action: (value: T, key: string) => void): void {
for (const key in data) {
action(data[key], key);
}
}
}
}I bet this is allocating a different closure for each method, wheras a class just allocates space for data and shares a single prototype among all instances.
| // Map utilities | ||
| namespace ts { | ||
| /** Create a map containing a single entry key -> value. */ | ||
| export function createMapWithEntry<T>(key: string, value: T): Map<string, T> { |
There was a problem hiding this comment.
or using the Map constructor optional parameter
| } | ||
|
|
||
| // Map utilities | ||
| namespace ts { |
There was a problem hiding this comment.
should be marked internal. and consider using one closure for all of them
|
|
||
| // If output has not been changed, and the file has no external modification | ||
| if (fingerprint.byteOrderMark === writeByteOrderMark && | ||
| if (fingerprint !== undefined && fingerprint.byteOrderMark === writeByteOrderMark && |
added 6 commits
October 11, 2016 13:44
… string keys (which it did before too)
mhegazy
approved these changes
Oct 25, 2016
|
Can you also share the perf numbers once it is in. |
|
@rbuckton any more comments? |
rbuckton
approved these changes
Oct 27, 2016
| declare const Map: { new<K, V>(pairs?: [K, V][]): Map<K, V> } | undefined; | ||
| const usingNativeMaps = typeof Map !== "undefined"; | ||
| // tslint:disable-next-line:no-in-operator | ||
| const fullyFeaturedMaps = usingNativeMaps && "keys" in Map.prototype && "values" in Map.prototype && "entries" in Map.prototype; |
There was a problem hiding this comment.
The term "fully-featured" is a bit ambiguous. Maybe usingES6NativeMaps?
| * In runtimes without Maps, this is implemented using an object. | ||
| * `pairs` is an optional list of entries to add to the new map. | ||
| */ | ||
| export const StringMap: { new<T>(pairs?: [string, T][]): Map<string, T>; } = usingNativeMaps ? Map : ShimMap; |
There was a problem hiding this comment.
I'd love for us to be able to just call this Map instead of StringMap or NumberMap, but its a bit messy to avoid colliding with the global Map
There was a problem hiding this comment.
Or stick with createMap<K extends string | number, V>() and createSet<T extends string | number>()
| * Creates a sorted array of keys. | ||
| * Sorts keys according to the iteration order they would have if they were in an object, instead of from a Map. | ||
| * This is so that tests run consistently whether or not we have a Map shim in place. | ||
| * The difference between Map iteration order and V8 object insertion order is that V8 moves natrual-number-like keys to the front. |
| } | ||
|
|
||
| delete(value: string) { | ||
| delete this.data[value]; |
There was a problem hiding this comment.
Native Sets return boolean for delete.
| } | ||
|
|
||
| delete(key: K): void { | ||
| delete this.data[key as string]; |
There was a problem hiding this comment.
Native Maps return boolean for delete.
| However, `forEach` will iterate over strings because values are stringified before being put in the map. | ||
| */ | ||
|
|
||
| constructor(pairs?: [K, V][]) { |
There was a problem hiding this comment.
Non-ES6 native maps don't support constructor arguments.
rbuckton
reviewed
Oct 27, 2016
| delete(key: K): void { | ||
| delete this.data[key as string]; | ||
| delete(key: K): boolean { | ||
| const had = this.has(key); |
There was a problem hiding this comment.
No need for this. Just do return delete this.data[key as string].
There was a problem hiding this comment.
delete always returns true in strict mode.
There was a problem hiding this comment.
Ah. That's unfortunate...and odd.
rbuckton
reviewed
Oct 27, 2016
| delete(value: string) { | ||
| delete this.data[value]; | ||
| delete(value: string): boolean { | ||
| const had = this.has(value); |
There was a problem hiding this comment.
As above, you can just do return delete this.data[value] since a delete expression returns boolean
|
Will do a final performance comparison tomorrow. |
ghost
mentioned this pull request
ghost
pushed a commit
that referenced
this pull request
Oct 27, 2016
Revert "Merge pull request #11354 from Microsoft/map4"
mhegazy
added a commit
that referenced
this pull request
Nov 10, 2016
* Use a single ShimMap class, and indicate that iteration always yields string keys (which it did before too) * Remove emacs-added newline at end of baseline * Remove space at end of line in baseline * Add numeric indexer to strings and remove lint * Update baselines with spread string's numeric indexers * Reset baseline line number to the locally incorrect value * Remove trailing newline from baseline again * Respond to PR comments * Refactor getTypeFromTypeLiteral, from PR comments * Fix lint * Use same literal comparison rules for switch/=== switch was missing the rule for converting literal types to the literal base type if needed. * Add switch comparability test and update baselines * Push eitherIsNotLiteral check into isTypeEqualityComparableTo Since all callers need this check -- it's how equality is supposed to work everywhere. * Update baselines There are more literal types in error messages because error reporting no longer has the advantage of calls to getBaseLiteralType * Adds ES5 to ES3 transformer for reserved words * Minor cleanup * Cleaned up emit of enum declaration * Return both ts and js results from module resolution, and don't have moduleNameResolver responsible for omitting files based on compiler options * Instead of getResolutionOrDiagnostic, use getResolutionDiagnostic and avoid using resolution.resolvedFileName if the diagnostic is defined. * Remove "ResolvedModuleFromHost" type and just make `resolvedTsFileName` and `resolvedJsFileName` optional properties (but still automatically infer one of them to supply if the host supplied neither) * Remove type inference for spread types * Update inference test for spread types * Make spread assignability and apparent type stricter Assignability now does not allow properties to the left of a type parameter. Apparent type now only returns the apparent type of the right-most spread member. * Update tests w/spread assignability+apparent type * Moved AMD/CJS/UMD transform to end * Spread no longer distributes intersections * Update spread w/intersection tests * Move System module transform to end. * Update baseline * Revert baseline change due to stale output * Resolve all-object intersections inside spreads This means that getSpreadType will return an object type, even when spreading two intersections, as long as those intersections contain nothing but object types themselves. * Update and improve spread intersection tests * Simplify expression in resolveObjectIntersection * Update baselines with new index signature rules * Move n-ary spread handling into separate function. To be moved to callers in the next step. * Move multiple-spread handling out of getSpreadType * Clean up and reorder getSpreadType body * Respond to PR comments * Error for call/construct signatures in spread type 1. Simplify the error reporting code to handle all kinds of signatures. 2. Remove index signature handling code when creating a spread type since it's an error anyway. * Update baselines with new spread type index errors * Explain writeSpreadType a little better * Add more commentary to getSpreadType * Initial implementation of 'keyof T' type operator * Introduce PropertyNameType * Move most of resolveModuleNameForLsHost to lsHost * Simplify isImplicitGlob test * Initial implementation of 'T[K]' property access types * add a fallback logic for older versions of node that don't support 'homedir' (#11845) * add a fallback logic for older versions of node that don't support 'homedir' * try os.homedir first * For JavaScript files, we report semantic errors for using TypeScript-only constructs from within a JavaScript file as syntactic errors. * Don't require `resolvedTsFileName` and `resolvedJsFileName`, just `resolvedFileName` and `extension`. Also, change search order to do all TS searching before searching for JS. * Fix #10108 (Completion suggestion for object literal with getter) (#11808) * Fix #10108 (Completion suggestion for object literal with getter) * completions for setter * Move helper functions to core (fix build) * Support parametric property access expressions + some renaming * rewrite void-returning statements in constructors that capture result of super call (#11868) * rewrite void-returning statements in constructors that capture result of super call * linter * only emit /// types reference for a symbol in d.ts file if all declarations of a symbol come from type reference directives (#11872) * only emit /// types reference for a symbol in d.ts file if all declarations of a symbol come from type reference directives * pass proper value for current directory when compiling .d.ts files * Fix bug: Return a resolution diagnostic for a `.jsx` import if `--allowJs` is turned off * Remove a comment about a parameter that no longer exists * Fix bug: We want to test for existence of the enum value, not whether it's non-zero * Fix: test for presence, not absence * Allow untyped imports * Simplify for loops in fourslash.ts * Rename to zipWith * Change diagnostic message * Make `extension` property of `ResolvedModule` optional; introduce `ResolvedModuleFull` interface for when the extension is provided. * Consider index signatures in type produced by 'keyof T' * Respond to PR comments * Refactor getIndexedAccessType to be reusable from checkIndexedAccess * Skip overloads with too-short function parameters If the parameter of an overload is a function and the argument is also a function, skip the overload if the parameter has fewer arguments than the argument does. That overload cannot possibly apply, and should not participate in, for example, contextual typing. Example: ```ts interface I { (a: number): void; (b: string, c): void; } declare function f(i: I): void; f((x, y) => {}); ``` This code now skips the first overload instead of considering. This was a longstanding bug but was only uncovered now that more functions expressions are context sensitive. * Test skip overloads w/too-short function params 1. Update changed baseline. 2. Add a new test with baseline. * Minor style improvements * Ignore optionality when skipping overloads * Revert "Merge pull request #11354 from Microsoft/map4" This reverts commit adfdae0, reversing changes made to aad663c. * Rename Experimental transform to ESNext 1. Spread/rest are no longer experimental. 2. We need a place to put stage 3 ES features. * Changes from CR feedback * disable CoS for inferred projects (#11909) * Updating test due to CR changes. The order of the diagnostic messages has changed due to concatenation changes * Do not use contextual signatures with too few parameters * isAritySmaller runs later: getNonGenericSignature * Rename TransformFlags.Experimental -> ESNext * enable non-ts extensions in inferred projects by default * update CFG to properly handle do statements * do not inline async IIFEs in control flow graph * Minor fixes * cache type for empty type literal (#11934) cache type for empty type literal * Improved error messages for invalid assignments to identifiers * Accept new baselines * Improved error messages for invalid assignments to properties * Accept new baselines * Improve more error messages * Accept new baselines * Fix realPathMap in test harness: Must be used in `directoryExists` * Add helper function * Update generated files (#11963) * Move eitherIsNotLiteral check into switch and === checks This improves error messages * Forbid augmentation of untyped module (#11962) * Forbid augmentation of untyped module * Just use `undefined` for untyped modules * Unify checking of indexed access expressions and indexed access types * Accept new baselines * Accept additional baselines * Make `cachingInServerLSHost` tests work with `runtests-browser` * Get literal type only once * Update baselines * Add missed test update * Improve unification by moving more logic to getIndexedAccessType * Add test cases to report errors for decorators in js file * Report all the js file errors and skip only the nodes that are not allowed in js Fixes #11800 * Fix 'keyof any' to produce 'string | number' * Fix #11396: Make error message referene `Promise` explicitly (#11982) * Simplify the checking for async function return type * Fix #11396: Make error message referene `Promise` explicitly * Minor fixes * Adding tests * Test case for property used in destructuring variable declaration * Mark property referenced in the destructuring as referenced Fixes #11324 * Spread types handle nested index [access] types Nested index [access] types are treated as assignable to themselves only, just like type parameters. * Test index [access] types inside spread types * module resolution: prefer locally defined ambient modules, reuse resolutions to ambient modules from the old program (#11999) module resolution: prefer locally defined ambient modules, reuse resolutions to ambient modules from the old program * Parse, bind and check rest elements * Downlevel emit of rest elements * Add rest tests * Update baselines * Remove spread types, leaving spread syntax/emit Spreads are still typed, but cannot be created from a non-object type. Tests still need to be updated. * Fix lint * Lock tslint version to 4.0.0-dev.0, because 4.0.0-dev.1 complains about unnecessary semicolons following properties * Remove spread type tests from spread tests * Spread handles index signatures from singleton spreads I broke it when simplifying the logic earlier. * Correct assignability for keyof types and type parameters * Update tests * Accept new baselines * Update objectRestAssignment test and baselines * Fix linting errors * Update objectRestAssignment test Missed previously, just got the baselines * Improve readability of ES next destructuring emit * Ensure transformFlags are correct before visiting a node. * Port #12027, #11980 and #11932 to master (#12037) * add test for the fix for overwrite emitting error * cr feedback * Address PR comments 1. Remove extra line in __rest shim. 2. Improve __rest vs __assign check for destructuring assignment. * Move convertForOf to factory for esnext and es2015 Saves a lot of duplicated code * Update improved baselines * Move transformFunctionBody to factory It is shared by es2015 and esNext transformers. This commit just adds a convertObjectRest flag to be passed on to flattenDestructuring functions, as well as adding necessary parameters to use the code outside a transformer. * Spread any types to any * add missing bind calls to properly set parent on token nodes (#12057) * Cache generic signature instantiations * Accept new baselines * Properly instantiate aliasTypeArguments * Add regression test * Revert incorrect logic from #11392 * Accept new baselines * Rename SpreadElementExpression -> SpreadAssignment and SpreadExpression (formerly SpreadElementExpression) -> SpreadElement * Add SpreadAssignment to visitors 1. visitNode 2. reduceNode 3. emit This fixes an emit bug for setters. * Add --target esnext Currently, this disables the rest and spread transforms. This will change as proposals enter and leave stage 3. * Add --target esnext tests and update baselines * Revert unneeded change and comments per PR * Rename variable in checkSwitchStatement per PR * Do not emit __rest for --target esnext * Add `realpath` implementation for lshost * Create spread property types eagerly This avoids the need for a synthetic symbol and later code called from getTypeOfSymbol. * Set spread type symbols in checkObjectLiteral Instead of getSpreadType. Also clean up special-case handling inside getSpreadType to be more readable. * Ports #12051 and #12032 into master (#12090) * use local registry to check if typings package exist (#12014) use local registry to check if typings package exist * enable sending telemetry events to tsserver client (#12035) enable sending telemetry events * Address more PR comments * return empty file watcher in case if target directory does not exist (#12091) * return empty file watcher in case if target directory does not exist * linter * property handle missing config files in external projects (#12094) * Reuse subtree transform flags for incrementally parsed nodes (#12088) * Port fix for #12069 (#12095) * reduce set of files being watched, increase polling interval (#12054) (#12092) * Update authors for release-2.1 * Include declaration file emit * use createHash from ServerHost instead of explicit require (#12043) * use createHash from ServerHost instead of explicit require * added missing method in ServerHost stub * (signature help) type parameter lists are never variadic (#12112) * Handle abstract and const modifiers * Downlevel array destructuring to ES6 in object rest Previously array destructuring inside an object destructuring with an object rest would downlevel the array destructuring to ES5. This breaks if the code that targets ES2015 is using iterators instead of arrays since iterators don't support [0] or .slice that the ES5 emit uses. * Improve nested destructuring test for ESNext emit Now with object destructuring inside array destructuring inside object destructuring! Each with their own array/object rest! Also updates baselines. * Add support for taking in jsxFactory option and report errors for invalid combinations * Treat `| undefined` like optionality in spread type * Add strictNullChecks test for object spread * Verify that jsxFactory is either identifier or qualified name * Resolve first identifier of the jsxFactory as part of type check * When emitting use jsx factory entity expression if provided * Transpile unit test case * Enabled test case for source map * Add test cases for when jsxFactory cannot be resolved * Parse the jsxFactory again in the checker instead of using cached value in the program * Adds error message for incompatible assignment of identically named type Fixes issue #12050 * Updated condition for more readability * Report errors for import helpers missing __rest * Correctly check spread assignments in strict mode Previously it crashed in the binder. * Test error for import helpers with no __rest * synthesize complete import declaration for tslib (#12151) * Add ES2017 string padding (#12152) * add es2017.string.d.ts for String.prototype.{padStart,padEnd} * append es2017.string.d.ts into es2017.d.ts * add es2017.string into commandLineParser * append es2017.string into error message for unit tests of commandLineParser * append es2017.string into Gulpfile * append es2017.string into Jakefile * Exclude js files in non-configured projects compile-on-save emitting (#12118) * Exclude js files in non-configured projects CoS emitting * remove unnecessary method * Merge release-2.1 into master (#12157) * Update LKG * Update version * Update LKG * Skip overloads with too-short function parameters If the parameter of an overload is a function and the argument is also a function, skip the overload if the parameter has fewer arguments than the argument does. That overload cannot possibly apply, and should not participate in, for example, contextual typing. Example: ```ts interface I { (a: number): void; (b: string, c): void; } declare function f(i: I): void; f((x, y) => {}); ``` This code now skips the first overload instead of considering. This was a longstanding bug but was only uncovered now that more functions expressions are context sensitive. * Test skip overloads w/too-short function params 1. Update changed baseline. 2. Add a new test with baseline. * Minor style improvements * Ignore optionality when skipping overloads * Do not use contextual signatures with too few parameters * isAritySmaller runs later: getNonGenericSignature * rewrite void-returning statements in constructors that capture result of super call (#11868) * rewrite void-returning statements in constructors that capture result of super call * linter * Update LKG * Fix emit inferred type which is a generic type-alias both fully and partially fill type parameters * Add tests and baselines * Skip trying to use alias if there is target type * Update baselines * Add diagnostics to remind adding tsconfig file for certain external project (#11932) * Add diagnostics for certain external project * Show tsconfig suggestion * fix lint error * Address pr * fix comment * Update error message * Flag for not overwrite js files by default without generating errors (#11980) * WIP * Properly naming things * refactor * apply the option to all files and check out options * Fix typo * Update LKG * lockLinter * use local registry to check if typings package exist (#12014) (#12032) use local registry to check if typings package exist * Add test for #11980 (#12027) * add test for the fix for overwrite emitting error * cr feedback * enable sending telemetry events to tsserver client (#12034) (#12051) enable sending telemetry events * Update LKG * Reuse subtree transform flags for incrementally parsed nodes (#12088) * Update LKG * Update version * Update LKG * Do not emit "use strict" when targeting es6 or higher or module kind is es2015 and the file is external module * Add tests and baselines * [Release 2.1] fix11754 global augmentation (#12133) * Exclude global augmentation from module resolution logic * Address PR: check using string literal instead of NodeFlags.globalAugmentation * Remove comment
ghost
mentioned this pull request
This pull request was closed.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
The summary is that using native maps, the compiler works 5% faster and uses 5% less memory. Using shims, the performance is surprisingly less than 5% worse.
I was able to achieve good baseline performance by disabling Windows Defender in my work directory and using "High Performance" in Control Panel -> Hardware and Sound -> Power Options. There is much less variance this time around.
Benchmark numbers
This is from using our internal
ts-perftool.map4
Monaco
tsc (x86)
node (v6.7.0, x64)
node (v6.7.0, x86)
TFS
tsc (x86)
node (v6.7.0, x64)
node (v6.7.0, x86)
map4 with
usingNativeMaps = falseandusingNativeSets = falseMonaco
tsc (x86)
node (v6.7.0, x64)
node (v6.7.0, x86)
TFS
tsc (x86)
node (v6.7.0, x64)
node (v6.7.0, x86)
Heap snapshot numbers
The memory usage tended to be about the same up until the
beforeEmitevent, AKA after checking.Master had 365,935 KB total, map4 had 343,774 KB (94% as much), and map4 with shims had 368,978 KB (100% as much as master, 106% as much as map4).
(I added an event
heapAfterBindto test the effects of that; map4 used 97% as much memory as master, meaning the real memory savings are during checking.)For map4, the chrome heap snaprhot viewer shows Maps as accounting for 2% of all objects, with a shallow size equal to their retained size at 1%. However, this is misleading because most of them are mere 32KB wrapper objects which have internal
_keysand_values(ortablein some cases), which is what really takes up space. I wish there were a way we could include the size of those.VSCode numbers
I also ran a simple test on the VSCode repo.
master
tsc -p src --noEmit --diagnostics(Run with2.1.0-dev.20161005)map4
node ../TypeScript/built/local/tsc.js -p src --noEmit --diagnosticsmap4 with shims
Same command as for
map4, after rebuilding withusingNativeMaps = falseandusingNativeSets = false.