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

use proposer index cache for block proposal #4338

Closed
wants to merge 3 commits into from
Closed

Conversation

int88
Copy link
Contributor

@int88 int88 commented May 26, 2023

Issue Addressed

#4313

Proposed Changes

use proposer index cache for block proposal

Additional Info

NA

@int88
Copy link
Contributor Author

int88 commented May 26, 2023

@michaelsproul PTAL, I don't know if I handle some details properly 😂

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.

Looks good!

I'm planning to do some benchmarking of this PR with your other optimisation next week 🔜

Comment on lines 4324 to 4326
let (proposers, decision_root, _, fork) =
compute_proposer_duties_from_head(proposer_epoch, self)
.map_err(BlockProductionError::BeaconChain)?;
Copy link
Member

Choose a reason for hiding this comment

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

I think we could simplify this a little by calling state.get_beacon_proposer_indices on the state directly. It has already been advanced through to the slot of the proposal (above) so it should be able to serve the call.

The compute_proposer_duties_from_head basically does the same thing except it also clones the head again (which is slow). Using the head is also not necessarily correct if we are proposing on a non-head block (sometimes in tests we propose blocks using an old parent instead of the head).

The fork can be set to state.fork() directly, and we also have the dependent_root (same as the decision root) above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelsproul updated! But delete some proposer cache prime check in some tests to pass CI.

@michaelsproul michaelsproul self-assigned this May 30, 2023
@michaelsproul michaelsproul added waiting-on-author The reviewer has suggested changes and awaits thier implementation. optimization Something to make Lighthouse run more efficiently. labels May 30, 2023
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jun 1, 2023
@michaelsproul
Copy link
Member

Just a quick update for you @int88, I did some benchmarking of this PR in combination with #4337 and #4328 and unfortunately it increased the median time by around 9ms, from 437ms to 446ms. I'm looking into why, weirdly it doesn't seem to be caused by the proposer cache missing

@michaelsproul
Copy link
Member

Closing this for now as it doesn't seem to be a promising avenue for optimisation. We can revisit later if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimization Something to make Lighthouse run more efficiently. ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants