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

[Merged by Bors] - Use forwards iterator for state root lookups #2422

Closed
wants to merge 5 commits into from

Conversation

macladson
Copy link
Member

@macladson macladson commented Jun 28, 2021

Issue Addressed

#2377

Proposed Changes

Implement the same code used for block root lookups (from #2376) to state root lookups in order to improve performance and reduce associated memory spikes (e.g. from certain HTTP API requests).

Additional Changes

  • Tests using rev_iter_state_roots and rev_iter_block_roots have been refactored to use their forwards versions instead.
  • The rev_iter_state_roots and rev_iter_block_roots functions are now unused and have been removed.
  • The state_at_slot function has been changed to use the forwards iterator.

Additional Info

  • Some tests still need to be refactored to use their forwards_iter versions. These tests start their iteration from a specific beacon state and thus use the rev_iter_state_roots_from and rev_iter_block_roots_from functions. If they can be refactored, those functions can also be removed.

@macladson macladson added the work-in-progress PR is a work-in-progress label Jun 28, 2021
@macladson
Copy link
Member Author

The improvements from this change can be seen in the graph below:

forwards_iter

I made three requests to the get_beacon_states_committees API endpoint requesting historical beacon states.
Left: stable Lighthouse v1.4

  • Memory usage spikes to nearly 2.5GB and this memory increase seems everlasting (although it seems available to the rest of the system).

I then relaunched Lighthouse with the changes and made the same three requests.
Right: After changes

  • No spike occurs

@macladson macladson changed the title Use forward iterators for state root lookups Use forwards iterator for state root lookups Jun 29, 2021
@macladson macladson marked this pull request as ready for review July 1, 2021 03:25
@macladson macladson added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Jul 1, 2021
@michaelsproul michaelsproul self-requested a review July 5, 2021 14:18
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

This looks great!

I think we can merge this before altair, and then I'll sort out the merge conflicts

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jul 6, 2021
@michaelsproul
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jul 6, 2021
## Issue Addressed

#2377 

## Proposed Changes

Implement the same code used for block root lookups (from #2376) to state root lookups in order to improve performance and reduce associated memory spikes (e.g. from certain HTTP API requests).

## Additional Changes

- Tests using `rev_iter_state_roots` and `rev_iter_block_roots` have been refactored to use their `forwards` versions instead.
- The `rev_iter_state_roots` and `rev_iter_block_roots` functions are now unused and have been removed.
- The `state_at_slot` function has been changed to use the `forwards` iterator.

## Additional Info

- Some tests still need to be refactored to use their `forwards_iter` versions. These tests start their iteration from a specific beacon state and thus use the `rev_iter_state_roots_from` and `rev_iter_block_roots_from` functions. If they can be refactored, those functions can also be removed.
@bors bors bot changed the title Use forwards iterator for state root lookups [Merged by Bors] - Use forwards iterator for state root lookups Jul 6, 2021
@bors bors bot closed this Jul 6, 2021
@macladson macladson deleted the forwards-iter-state-roots branch July 6, 2021 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants