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

[store] use hasCache to minimize pendingRef pool #7672

Merged
merged 4 commits into from
Apr 2, 2024

Conversation

max-hoffman
Copy link
Contributor

@max-hoffman max-hoffman commented Apr 1, 2024

Expand the ChunkStore interface to let the nodeStore access recently accessed chunks. Avoid adding a child ref to the pendingRef list when already present in nbs.hasCache. For TPC-C this appears to reduce the pending ref count by another ~80%.

@max-hoffman
Copy link
Contributor Author

#benchmark

Copy link

github-actions bot commented Apr 1, 2024

@coffeegoddd
Copy link
Contributor

@max-hoffman DOLT

test_name from_latency_median to_latency_median is_faster
tpcc-scale-factor-1 118.92 118.92 0
test_name server_name server_version tps test_name server_name server_version tps is_faster
tpcc-scale-factor-1 dolt 053e6ca 17.63 tpcc-scale-factor-1 dolt 0fa773c 17.71 0

@coffeegoddd
Copy link
Contributor

@max-hoffman DOLT

comparing_percentages
100.000000 to 100.000000
version result total
0fa773c ok 5937457
version total_tests
0fa773c 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@max-hoffman DOLT

read_tests from_latency_median to_latency_median is_faster
covering_index_scan 2.97 2.91 0
groupby_scan 17.95 17.63 0
index_join 5.09 5.09 0
index_join_scan 2.26 2.22 0
index_scan 54.83 53.85 0
oltp_point_select 0.49 0.49 0
oltp_read_only 8.28 8.13 0
select_random_points 0.78 0.78 0
select_random_ranges 0.95 0.94 0
table_scan 54.83 54.83 0
types_table_scan 158.63 158.63 0
write_tests from_latency_median to_latency_median is_faster
oltp_delete_insert 6.91 6.79 0
oltp_insert 3.43 3.36 0
oltp_read_write 15.83 15.55 0
oltp_update_index 3.55 3.43 0
oltp_update_non_index 3.49 3.36 0
oltp_write_only 7.84 7.56 0
types_delete_insert 7.7 7.43 0

Copy link
Contributor

@reltuk reltuk left a comment

Choose a reason for hiding this comment

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

I know this was just testing perf impact...but for iterating towards landing it, I definitely don't think landing this with a new interface on the ChunkStore is the right direction.

Instead, I would think something like getAddrsCb becomes something like func(chunk []byte, func(h hash.Hash)) and then NomsBlockStore implementation uses that to actually build the hashset that it wants, and in the process filters by the concrete hasCache that was passed into tableSet.append or whatever.

@reltuk
Copy link
Contributor

reltuk commented Apr 2, 2024

If perf impact of the interface change is bad, even getAddrsCb as func(chunk []byte, insertIntoThis hash.HashSet, filterByThis Haser) type Haser interface { Has(hash.Hash) bool } or func(chunk []byte, insertIntoThis hash.HashSet, filterByThis lru...) or something could be preferable...

@max-hoffman
Copy link
Contributor Author

#benchmark

Copy link

github-actions bot commented Apr 2, 2024

@coffeegoddd
Copy link
Contributor

@max-hoffman DOLT

test_name from_latency_median to_latency_median is_faster
tpcc-scale-factor-1 121.08 118.92 0
test_name server_name server_version tps test_name server_name server_version tps is_faster
tpcc-scale-factor-1 dolt d6aa1e6 17.91 tpcc-scale-factor-1 dolt c832396 18.11 0

@coffeegoddd
Copy link
Contributor

@max-hoffman DOLT

comparing_percentages
100.000000 to 100.000000
version result total
212f07f ok 5937457
version total_tests
212f07f 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@max-hoffman DOLT

read_tests from_latency_median to_latency_median is_faster
covering_index_scan 3.02 2.97 0
groupby_scan 17.32 17.63 0
index_join 5.09 5.09 0
index_join_scan 2.26 2.22 0
index_scan 54.83 54.83 0
oltp_point_select 0.49 0.49 0
oltp_read_only 8.13 8.13 0
select_random_points 0.78 0.78 0
select_random_ranges 0.94 0.94 0
table_scan 54.83 54.83 0
types_table_scan 161.51 158.63 0
write_tests from_latency_median to_latency_median is_faster
oltp_delete_insert 6.91 6.67 0
oltp_insert 3.43 3.3 0
oltp_read_write 15.83 15.55 0
oltp_update_index 3.49 3.43 0
oltp_update_non_index 3.49 3.36 0
oltp_write_only 7.7 7.56 0
types_delete_insert 7.7 7.43 0

Copy link
Contributor

@reltuk reltuk left a comment

Choose a reason for hiding this comment

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

LGTM!

@max-hoffman max-hoffman merged commit 864f962 into main Apr 2, 2024
20 checks passed
@max-hoffman max-hoffman deleted the max/pending-ref-has-cache-check branch April 2, 2024 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants