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

Cache all memberNamed results #9633

Merged
merged 3 commits into from
Aug 27, 2020
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Aug 25, 2020

Use a full cache instead of an LRU cache. For typer/*.scala, this
reduced computed member searches on normal ClassDenotations from 520K to
170K. Cache hit rate improves to 97.5%, from 92.5%. (Without member caching
there are almost 7Mio computed members for the same code base).

@odersky
Copy link
Contributor Author

odersky commented Aug 25, 2020

Back of the envelope calculation for memory sizes. All numbers for typer/*.scala with Stats enabled, so numbers are a bit higher than without.

We have 174K computeNonPrivateMembersNamed

Caches: 4657 ArrayTables
1293 HashTables

Small TableSizes:

size #tables

0 1532
1 1689
2 564
3 366
4 200
5 175
6 120
7 121
8 90

Total entries in ArrayTables: 13788. This leaves ~160K entries in HashTables. So member accesses are spread out very unevenly. The majority is in relatively few large tables.

Largest 5 table sizes: 4079, 4045, 2353, 1443, 1400. Then there are 20 or so between 1000 and 1300. I suspect the two
largest sizes are for Any and Object.

Size overhead: ArrayTables: 4657 * 88 bytes = 410K, or 30 bytes per used entry
Size overhead: HashTables: approx. 30bytes/entry --> ~4.8M
Total ~5.2M

Previous solution with LRU caches:

5950 caches * 176 bytes ~ 1M

Bottom line: we get a 3x improvement in cache hit rate (from 92.5% to 97.5%) for a 5x increase in memory (1M -> 5.2M).

@odersky
Copy link
Contributor Author

odersky commented Aug 25, 2020

test performance please

@dottybot
Copy link
Member

performance test scheduled: 2 job(s) in queue, 1 running.

@odersky
Copy link
Contributor Author

odersky commented Aug 25, 2020

Downstream improvement: # allocated AsSeenFromMap goes from 124572 to 93616

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9633/ to see the changes.

Benchmarks is based on merging with master (c908b13)

@odersky
Copy link
Contributor Author

odersky commented Aug 25, 2020

test performance please

@dottybot
Copy link
Member

performance test scheduled: 4 job(s) in queue, 1 running.

@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9633/ to see the changes.

Benchmarks is based on merging with master (0246f72)

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@odersky odersky force-pushed the full-member-caching branch 2 times, most recently from 907f7a0 to cf6531d Compare August 26, 2020 15:53
@odersky
Copy link
Contributor Author

odersky commented Aug 26, 2020

test performance please

@odersky
Copy link
Contributor Author

odersky commented Aug 26, 2020

@nicolasstucki Can you help me debug the idempotency failure here? In particular, is there a way to repeoduce this in a standalone test? I cannot reproduce the failure locally, it's CI only.

@dottybot
Copy link
Member

performance test scheduled: 1 job(s) in queue, 1 running.

@odersky odersky force-pushed the full-member-caching branch 2 times, most recently from 00d16ac to bc1eaf3 Compare August 26, 2020 17:31
Use a full cache instead of an LRU cache. For `typer/*.scala`, this
reduced computed member searches on normal ClassDenotations from 520K to
170K. Cache hit rate improves to 97.5%, from 92.5%. (Without member caching
there are almost 7Mio computed members for the same code base).
The two implementation classes of LinearTable had so much in common that
they could be combined. Since the implementation is now fixed, we can
use the usual mutable interface for a table.
When going from dense to hashed, we should expand by a multiple greater
than two, or we will have to reallocate immediately afterwards.
@dottybot
Copy link
Member

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/9633/ to see the changes.

Benchmarks is based on merging with master (a036b70)

@odersky
Copy link
Contributor Author

odersky commented Aug 26, 2020

The recent deterioration is due to parallel position pickling being disabled now.

@odersky odersky merged commit 8ba1ea0 into scala:master Aug 27, 2020
@odersky odersky deleted the full-member-caching branch August 27, 2020 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants