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

Add StatusCache::root_slot_deltas() and use it #26170

Merged
merged 2 commits into from
Jun 23, 2022

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Jun 23, 2022

Problem

All uses of StatusCacheRc::slot_deltas() (1) only get the roots from the status cache, and (2) unnecessarily sort. These are performance loses.

Summary of Changes

  1. Add StatusCache::root_slot_deltas() which gets the slot deltas from its roots
  2. Remove StatusCacheRc::slot_deltas() and call StatusCache::root_slot_deltas() instead
  3. Add benchmarks to ensure this is actually a speedup
1. test bench_status_cache_root_slot_deltas               ... bench:       8,366 ns/iter (+/- 592)
2. test bench_status_cache_slot_deltas                    ... bench:       9,200 ns/iter (+/- 612)
3. test bench_status_cache_slot_deltas_get_roots_sorted   ... bench:      16,086 ns/iter (+/- 2,978)
4. test bench_status_cache_slot_deltas_get_roots_unsorted ... bench:      10,692 ns/iter (+/- 809)

In (2), that is the current call to StatusCache::slot_deltas(). However, no code just calls (2); they all call StatusCache::roots() first. So (3) and (4) show the true results for the current way we get the slot deltas (really just (3)). And (1) is the new way.

The existing code performed a sort on the roots passed into slot_deltas(), but this is unnecessary. The vector returned is never indexed; in fact, it's only used for the snapshot, and its deserialization puts the vector back into a HashMap.

Getting the slot deltas is called during bank_forks::set_root(), so anything to speed that path up is useful.

Note: benches 3 and 4 are not part of the final PR. They are in the first commit, should you like to look at them and/or run them.

@brooksprumo brooksprumo force-pushed the status-cache/slot-deltas branch from c695fc5 to 7ab09e3 Compare June 23, 2022 03:54
@brooksprumo brooksprumo force-pushed the status-cache/slot-deltas branch from 7ab09e3 to 033fe6b Compare June 23, 2022 14:39
@brooksprumo brooksprumo marked this pull request as ready for review June 23, 2022 16:18
@codecov
Copy link

codecov bot commented Jun 23, 2022

Codecov Report

Merging #26170 (033fe6b) into master (1165a7f) will increase coverage by 0.0%.
The diff coverage is 90.6%.

@@           Coverage Diff           @@
##           master   #26170   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         631      631           
  Lines      174252   174247    -5     
=======================================
+ Hits       142728   142767   +39     
+ Misses      31524    31480   -44     

@brooksprumo brooksprumo requested review from carllin and HaoranYi June 23, 2022 16:18
Copy link
Contributor

@HaoranYi HaoranYi 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 23c50a2 into solana-labs:master Jun 23, 2022
@brooksprumo brooksprumo deleted the status-cache/slot-deltas branch June 23, 2022 20:19
gregcusack pushed a commit to gregcusack/solana that referenced this pull request Jun 23, 2022
gregcusack pushed a commit that referenced this pull request Jun 29, 2022
* add three gossip metrics measuring gossip loop times

* add 5 metrics

* rm space

* rm space

* Update SECURITY.md

- fix nav link
- add bounty split policy for duplicate reports

* Add transaction index in slot to geyser plugin TransactionInfo (#25688)

* Define shuffle to prep using same shuffle for multiple slices

* Determine transaction indexes and plumb to execute_batch

* Pair transaction_index with transaction in TransactionStatusService

* Add new ReplicaTransactionInfoVersion

* Plumb transaction_indexes through BankingStage

* Prepare BankingStage to receive transaction indexes from PohRecorder

* Determine transaction indexes in PohRecorder; add field to WorkingBank

* Add PohRecorder::record unit test

* Only pass starting_transaction_index around PohRecorder

* Add helper structs to simplify test DashMap

* Pass entry and starting-index into process_entries_with_callback together

* Add tx-index checks to test_rebatch_transactions

* Revert shuffle definition and use zip/unzip

* Only zip/unzip if randomize

* Add confirm_slot_entries test

* Review nits

* Add type alias to make sender docs more clear

* Update SECURITY.md

finish filling out the table....

* rpc: fix possible deadlock in rpc (#26051)

* Add StatusCache::root_slot_deltas() and use it (#26170)

* Remove InMemAccountsIndex::map() and use map_internal directly (#26189)

* [quic]Decrement total_streams correctly (#26158)

* remove comment

* alphabetical metrics. no abbreviations

* remove trailing white space

* cargo fmt to update code format/readability

Co-authored-by: Trent Nelson <trent@solana.com>
Co-authored-by: Tyera Eulberg <tyera@solana.com>
Co-authored-by: Boqin Qin(秦 伯钦) <Bobbqqin@gmail.com>
Co-authored-by: Brooks Prumo <brooks@solana.com>
Co-authored-by: Miles Obare <bdhobare@gmail.com>
@brooksprumo brooksprumo mentioned this pull request Jul 27, 2022
2 tasks
@ryoqun
Copy link
Member

ryoqun commented Aug 4, 2022

@brooksprumo hey, coming from #26666, i have some comments here. :)

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