Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Remove fn slot_deltas() from StatusCache #26931

Merged
merged 2 commits into from
Aug 16, 2022

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Aug 4, 2022

Problem

The slot_deltas() and root_slot_deltas() functions have different behavior w.r.t. not-found slots. They probably shouldn't.

Please see #26170 (comment) for more context.

Summary of Changes

Remove fn slot_deltas()

OLD SUMMARY BELOW

Summary of Changes

Add a new private function that wraps the impl of both slot_deltas() and root_slot_deltas(). Now both have the same behavior (to create a unique entry per missing slot, instead of sharing a single 'empty').

Some benchmarks:

before:

test bench_status_cache_root_slot_deltas  ... bench:       7,712 ns/iter (+/- 388)
test bench_status_cache_slot_deltas       ... bench:       8,826 ns/iter (+/- 1,448)
test bench_status_cache_slot_deltas_empty ... bench:       5,441 ns/iter (+/- 318)

after:

test bench_status_cache_root_slot_deltas  ... bench:       7,596 ns/iter (+/- 344)
test bench_status_cache_slot_deltas       ... bench:       8,582 ns/iter (+/- 539)
test bench_status_cache_slot_deltas_empty ... bench:      27,687 ns/iter (+/- 2,144)

The only change is that 'empty'/missing slots are now more expensive, since a new Status is created for each one. In practice I cannot imagine that roots have zero transactions, therefore this pathological case should never occur.

@brooksprumo brooksprumo force-pushed the status-cache/slot_deltas branch from e4831a4 to f24a5ee Compare August 4, 2022 23:57
@brooksprumo brooksprumo marked this pull request as ready for review August 5, 2022 03:11
@brooksprumo brooksprumo requested a review from ryoqun August 5, 2022 03:11
runtime/src/status_cache.rs Outdated Show resolved Hide resolved
runtime/src/status_cache.rs Outdated Show resolved Hide resolved
runtime/src/status_cache.rs Outdated Show resolved Hide resolved
@ryoqun
Copy link
Contributor

ryoqun commented Aug 8, 2022

I think the general direction is so good. Discussed elsewhere, i think this pr can further be smaller? could you rework? (sorry for wasted efforts...)

@brooksprumo brooksprumo changed the title Refactor status cache slot_deltas() and root_slot_deltas() Remove fn slot_deltas() from StatusCache Aug 8, 2022
@brooksprumo brooksprumo requested a review from ryoqun August 8, 2022 14:12
@brooksprumo
Copy link
Contributor Author

@ryoqun This PR is ready for another review. Thanks!

Copy link
Contributor

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

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

lgtm

@brooksprumo brooksprumo merged commit bc0d011 into solana-labs:master Aug 16, 2022
@brooksprumo brooksprumo deleted the status-cache/slot_deltas branch August 16, 2022 10:08
xiangzhu70 pushed a commit to xiangzhu70/solana that referenced this pull request Aug 17, 2022
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.

2 participants