Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use native maps where possible #11210

Closed
wants to merge 1 commit into from
Closed

Use native maps where possible #11210

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Sep 28, 2016

Note: This code needs a lot of cleanup; posted this as soon as I could get good performance numbers. I'll refactor it later if it looks like we will go ahead with this. Tests pass; travis is failing in the linter.

Memory performance numbers are lot more stable than time, and show a consistent 5% benefit.
The code without maps uses the same amount of memory, because we do not use a shim: we use a dictionary-mode object directly.

It works like this:

export const createMap: <T>() => Map<T> = realMaps
    ? <T>() => new Map<T>()
    : createDictionaryModeObject;

With similar conditions on every map-using function. e.g.:

//will rename this later.
export const _g: <T>(map: Map<T>, key: string) => T = realMaps
    ? <T>(map: NativeMap<T>, key: string) => map.get(key)
    : <T>(map: MapLike<T>, key: string) => map[key];

Map is an interface containing only a brand, so it can't be used directly, only through these functions. It's like an abstract data type.

Performance

Tested with the vscode source code.

Current

(tsc -p src --noEmit --diagnostics where tsc --version is 2.1.0-dev.20160928)

Measured Average Samples
Memory used 721556 723926, 717774, 722967
Parse time 7.18 6.84, 7.25, 7.44
Bind time 2.38 2.02, 2.59, 2.52
Check time 12.16 10.52, 11.96, 14.00
Total time 21.71 19.37, 21.81, 23.95

I use --noEmit because the emit phase is dominated by I/O write time.

Native maps

Measured Average Relative Samples
Memory used 683115 95% 677958, 684812, 686574
Parse time 6.98 97% 6.79, 7.23, 6.92
Bind time 1.97 83% 1.67, 2.16, 2.08
Check time 11.11 91% 10.60, 11.65, 11.09
Total time 20.06 92% 19.06, 21.03, 20.09

New code without native maps

(Latest version of node, running this branch with the code changed to disable using native maps.)

Measured Average Relative Samples
Memory used 725144 100% 716993, 728782, 729658
Parse time 7.66 107% 7.58, 6.83, 8.57
Bind time 2.14 90% 2.03, 2.13, 2.27
Check time 11.69 96% 11.63, 11.45, 11.99
Total time 21.49 99% 21.23, 20.41, 22.83

@ghost
Copy link
Author

ghost commented Oct 4, 2016

Closing in favor of #11354, which uses maps directly (using infix map.get(key) syntax) rather than through an abstract data type.

@ghost ghost closed this Oct 4, 2016
@mhegazy mhegazy deleted the map3 branch November 2, 2017 21:05
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant