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

perf(core): use BTreeMap for ResourceTable #10074

Merged
merged 5 commits into from
Apr 9, 2021

Conversation

AaronO
Copy link
Contributor

@AaronO AaronO commented Apr 8, 2021

Similar to #10073, this replaces ResourceTable's internal HashMap with a BTreeMap.

Rationale

This follows a similar rationale to #10073, improved performance on "small" sets of integer keys. However I've split the PRs since their keys have different distributions and therefor different performance profiles. ResourceTable has higher key churn and likely a higher key-count in practice than OpState.

Benches

Before (main):
date_now:            	n = 500000, dt = 0.029s, r = 17241379/s, t = 58ns/op
perf_now:            	n = 500000, dt = 0.218s, r = 2293578/s, t = 436ns/op
url_parse:           	n = 50000, dt = 0.325s, r = 153846/s, t = 6500ns/op
read_zero:           	n = 500000, dt = 0.328s, r = 1524390/s, t = 656ns/op
write_null:          	n = 500000, dt = 0.324s, r = 1543210/s, t = 648ns/op

After (BTree ResourceTable):
date_now:            	n = 500000, dt = 0.030s, r = 16666667/s, t = 59ns/op
perf_now:            	n = 500000, dt = 0.218s, r = 2293578/s, t = 436ns/op
url_parse:           	n = 50000, dt = 0.323s, r = 154799/s, t = 6460ns/op
read_zero:           	n = 500000, dt = 0.313s, r = 1597444/s, t = 626ns/op
write_null:          	n = 500000, dt = 0.313s, r = 1597444/s, t = 626ns/op

Future improvements

ResourceId distribution is very similar to PromiseId distribution (for apps that use resources), sequential integer keys with a practical sliding window of key relevance. So we could back the ResourceTable with a ring + fallback map design, which would likely be significantly faster than either HashMap or BTreeMap solo.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 0fd1fb9 into denoland:main Apr 9, 2021
@AaronO AaronO deleted the perf/btree-resource-table branch August 27, 2021 18: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.

None yet

4 participants