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 EnableNextSlotStateCache few more places #8398

Merged
merged 3 commits into from
Feb 8, 2021

Conversation

terencechain
Copy link
Member

What type of PR is this?

Feature

What does this PR do? Why is it needed?
Use #8357 in a few more places. Gated by the feature flag --enable-next-slot-state-cache
Which issues(s) does this PR fix?
N/A
Other notes for review
N/A

Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

Is it possible to do a bit of refactoring here and make all this logic under ProcessSlots ? That way we dont have to use this if else pattern everywhere and instead can just use it in one place. Also makes our code cleaner

@terencechain
Copy link
Member Author

Is it possible to do a bit of refactoring here and make all this logic under ProcessSlots ? That way we dont have to use this if else pattern everywhere and instead can just use it in one place. Also makes our code cleaner

I tried. It's actually not a clean refactor. Reasons:
1.) We'll have to add root to ProcessSlots's arguments. There's quite a few usages that don't have access to root and don't care about it
2.) In order to get root from the less relevant functions, we'll either need to read from db, or cache, or HTR something, I'm afraid of introducing further runtime bottlenecks or bugs.

I'm leaning towards leave it as it is

@terencechain terencechain merged commit fed6512 into develop Feb 8, 2021
@delete-merged-branch delete-merged-branch bot deleted the better-process-slots branch February 8, 2021 21:11
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