Skip to content

Reduce cache overhead #19163

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.

Sign up for GitHub

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

Jump to bottom
Closed
weswigham wants to merge 3 commits into from
Closed

Reduce cache overhead #19163

weswigham wants to merge 3 commits into from

Conversation

@weswigham
Copy link
Member

@weswigham weswigham commented Oct 13, 2017 •
edited
Loading

In this comment, I supposed that if we were concerned about the overhead of NodeLinks, we could reduce or remove it by inverting the shape of our cache structure. This is that.

This comparison was captured over 10 iterations on my machine (with a scanner exception for node and forced high power mode, as is usual for a windows machine):

Metric master reduce-cache-overhead Delta Best Worst
Compiler - Unions - node (v8.3.0, x64)
Memory used 203,010k (± 0.04%) 195,375k (± 0.04%) -7,636k (- 3.76%) 195,145k 195,530k
Parse Time 0.76s (± 3.91%) 0.73s (± 1.76%) -0.02s (- 3.03%) 0.72s 0.78s
Bind Time 0.66s (± 2.88%) 0.65s (± 0.72%) -0.01s (- 2.11%) 0.64s 0.66s
Check Time 10.19s (± 2.70%) 9.84s (± 1.81%) -0.35s (- 3.45%) 9.72s 10.54s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 11.61s (± 2.55%) 11.22s (± 1.56%) -0.39s (- 3.36%) 11.08s 11.91s
Monaco - node (v8.3.0, x64)
Memory used 331,677k (± 0.01%) 320,180k (± 0.02%) -11,497k (- 3.47%) 320,012k 320,301k
Parse Time 1.75s (± 1.32%) 1.73s (± 1.49%) -0.02s (- 1.03%) 1.70s 1.83s
Bind Time 0.70s (± 1.72%) 0.69s (± 1.22%) -0.01s (- 0.86%) 0.68s 0.71s
Check Time 3.69s (± 0.98%) 3.67s (± 1.16%) -0.02s (- 0.54%) 3.60s 3.79s
Emit Time 2.39s (± 1.20%) 2.38s (± 1.72%) -0.02s (- 0.75%) 2.31s 2.53s
Total Time 8.53s (± 0.73%) 8.47s (± 0.68%) -0.06s (- 0.73%) 8.37s 8.63s
TFS - node (v8.3.0, x64)
Memory used 288,481k (± 0.02%) 274,252k (± 0.02%) -14,229k (- 4.93%) 274,172k 274,334k
Parse Time 1.26s (± 1.99%) 1.24s (± 1.23%) -0.02s (- 1.19%) 1.23s 1.30s
Bind Time 0.71s (± 2.84%) 0.70s (± 0.68%) -0.01s (- 1.54%) 0.69s 0.71s
Check Time 3.25s (± 1.83%) 3.18s (± 0.96%) -0.07s (- 2.18%) 3.13s 3.27s
Emit Time 2.23s (± 2.55%) 2.11s (± 0.86%) -0.11s (- 4.99%) 2.07s 2.15s
Total Time 7.46s (± 1.86%) 7.25s (± 0.64%) -0.21s (- 2.79%) 7.15s 7.37s

There is consistently lower memory usage (since just add entries to spare arrays rather than allocate objects), and possible check time savings, too (since we no longer use a polymorphic NodeLinks object).

I'll try applying the same technique to SymbolLinks and see what the improvement there is.

@weswigham
Copy link
Member Author

weswigham commented Oct 13, 2017

Moving symbol links to the same layout yields slight further improvements:

Metric master reduce-cache-overhead Delta Best Worst
Compiler - Unions - node (v8.3.0, x64)
Memory used 203,010k (± 0.04%) 194,208k (± 0.03%) -8,802k (- 4.34%) 194,075k 194,315k
Parse Time 0.76s (± 3.91%) 0.73s (± 1.03%) -0.02s (- 3.17%) 0.72s 0.75s
Bind Time 0.66s (± 2.88%) 0.64s (± 1.29%) -0.02s (- 2.86%) 0.63s 0.66s
Check Time 10.19s (± 2.70%) 9.79s (± 0.52%) -0.40s (- 3.94%) 9.70s 9.93s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 11.61s (± 2.55%) 11.17s (± 0.47%) -0.44s (- 3.82%) 11.08s 11.31s
Monaco - node (v8.3.0, x64)
Memory used 331,677k (± 0.01%) 316,668k (± 0.02%) -15,009k (- 4.53%) 316,528k 316,768k
Parse Time 1.75s (± 1.32%) 1.73s (± 0.26%) -0.02s (- 1.37%) 1.72s 1.74s
Bind Time 0.70s (± 1.72%) 0.69s (± 1.28%) -0.00s (- 0.43%) 0.68s 0.71s
Check Time 3.69s (± 0.98%) 3.66s (± 0.50%) -0.03s (- 0.84%) 3.61s 3.70s
Emit Time 2.39s (± 1.20%) 2.33s (± 0.71%) -0.06s (- 2.46%) 2.29s 2.37s
Total Time 8.53s (± 0.73%) 8.41s (± 0.27%) -0.12s (- 1.38%) 8.37s 8.47s
TFS - node (v8.3.0, x64)
Memory used 288,481k (± 0.02%) 271,032k (± 0.02%) -17,450k (- 6.05%) 270,839k 271,116k
Parse Time 1.26s (± 1.99%) 1.24s (± 0.52%) -0.02s (- 1.27%) 1.23s 1.25s
Bind Time 0.71s (± 2.84%) 0.71s (± 1.16%) 0.00s ( 0.00%) 0.70s 0.74s
Check Time 3.25s (± 1.83%) 3.20s (± 1.57%) -0.06s (- 1.75%) 3.09s 3.28s
Emit Time 2.23s (± 2.55%) 2.12s (± 0.98%) -0.10s (- 4.54%) 2.07s 2.16s
Total Time 7.46s (± 1.86%) 7.28s (± 0.91%) -0.18s (- 2.37%) 7.12s 7.41s

I also tried removing TransientSymbol (not committed here) (which was a bit more work, as I had to remove all direct references to transient symbol fields) and did see some better perf, but worse memory usage (so a tradeoff - less polymorphism, but more space required to store the information).

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jun 12, 2018

Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it.

@weswigham weswigham mentioned this pull request Nov 7, 2018
Closed
@weswigham weswigham deleted the reduce-cache-overhead branch November 7, 2018 23:24
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rbuckton rbuckton Awaiting requested review from rbuckton

@ahejlsberg ahejlsberg Awaiting requested review from ahejlsberg

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@weswigham @typescript-bot