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

Flip build-block-parallel feature flag #12408

Merged
merged 2 commits into from
May 16, 2023

Conversation

terencechain
Copy link
Member

build-block-parallel is a great optimization for proposer, it has been tested on testnet over the last 2 weeks.
The feature was enabled in v4.0.4 and we are planning to make it default for v4.0.5 with a disable option.

@terencechain terencechain added the Ready For Review A pull request ready for code review label May 16, 2023
@terencechain terencechain requested a review from a team as a code owner May 16, 2023 02:00
@terencechain terencechain self-assigned this May 16, 2023
@terencechain terencechain requested review from saolyn, potuz and rkapka May 16, 2023 02:00
Comment on lines +217 to +220
cfg.BuildBlockParallel = true
if ctx.IsSet(disableBuildBlockParallel.Name) {
logEnabled(disableBuildBlockParallel)
cfg.BuildBlockParallel = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we not rename this to DisableBuildBlockParallel, that way it is false by default and then just set it to true if the flag disableBuildBlockParallel is set? Would also have to invert the condition where it is used in the code as well. But basically:

Suggested change
cfg.BuildBlockParallel = true
if ctx.IsSet(disableBuildBlockParallel.Name) {
logEnabled(disableBuildBlockParallel)
cfg.BuildBlockParallel = false
if ctx.IsSet(disableBuildBlockParallel.Name) {
logEnabled(disableBuildBlockParallel)
cfg.DisableBuildBlockParallel = true

It seems clearer than disableBuildBlockParallel setting BuildBlockParallel to false

Copy link
Member Author

Choose a reason for hiding this comment

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

But then I will have to replace DisableBuildBlockParallel with BuildBlockParallel on the implementation side which is not ideal. I rather keep the changes constrained in the flag package to avoid core changes risk

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been the standard pattern: #9225

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood, thank you for clarifying!

@terencechain terencechain merged commit 29f6de1 into develop May 16, 2023
@terencechain terencechain deleted the invert-build-block-parallel branch May 16, 2023 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants