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 a defaultReapInterval setting to active swingset configurations #5884

Merged
merged 1 commit into from
Aug 3, 2022

Conversation

FUDCo
Copy link
Contributor

@FUDCo FUDCo commented Aug 3, 2022

Issue #4160 set out to address the performance impact of Bring Out Your Dead, in particular to figure out what the optimal frequency should be set to. @arirubinstein ran a bunch of benchmarks that suggest that a value around 1000 was about right. What remained was to actually translate this into practice, which this PR does.

There remained an open question whether the appropriate way to set this value was to insert parameter settings into the configuration files for the swingsets that would be impacted operationally or to adjust the default value that's coded into SwingSet itself. After discussion, @warner and I concluded that the correct answer is to set it in the config files. There are two main reasons for this, one minor and one more significant.

The minor reason is that many, many tests implicitly depend on the existing default (which is 1) for the reference behavior that they check for. Updating the default in the kernel code would require all those tests to be identified and changed to set the parameter themselves. However, convenience of test maintenance is not by itself a very compelling reason for this course.

The more significant reason is that this parameter is one of the settings that can be dynamically adjusted at run time by governance action and it impacts the consensus behavior of the chain, so it is appropriate that it be managed explicitly rather than simply accepting whatever default the current SwingSet implementation happens to cough up. In particular, if we determine at a later time that a different source default is better, it would be best if the operational setting did not derive from something hardcoded into the kernel and thus cause on-chain behavior to alter just because of a perhaps unrelated kernel update. In other words, even if this PR changed the default in the kernel source code, we would still want to put it into the operational swingset configurations anyway. But since we are putting it there, in operational terms the kernel default is irrelevant and so it might as well stay with the value that's convenient.

Closes #4160

Note to @dckc: @warner and I guessed that you're the right person to pass judgement on this so you are tagged as the reviewer, but if you think otherwise, please feel free to decline it or kick it over to whoever you think might be more appropriate.

@FUDCo FUDCo added enhancement New feature or request SwingSet package: SwingSet performance Performance related issues hygiene Tidying up around the house Governance Governance solo the solo node (packages/solo) labels Aug 3, 2022
@FUDCo FUDCo requested a review from dckc August 3, 2022 01:32
@FUDCo FUDCo self-assigned this Aug 3, 2022
Copy link
Member

@dckc dckc left a comment

Choose a reason for hiding this comment

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

looks like a reasonable approach.

I haven't done anything to confirm that it works; I suppose we'll find out soon enough.

@FUDCo FUDCo added the automerge:rebase Automatically rebase updates, then merge label Aug 3, 2022
@mergify mergify bot merged commit 15234fa into master Aug 3, 2022
@mergify mergify bot deleted the 4160-set-default-reap-interval branch August 3, 2022 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge enhancement New feature or request Governance Governance hygiene Tidying up around the house performance Performance related issues solo the solo node (packages/solo) SwingSet package: SwingSet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance impact of BOYD
2 participants