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

Split compute budget instructions process from struct itself #33513

Conversation

tao-stones
Copy link
Contributor

@tao-stones tao-stones commented Oct 3, 2023

Problem

struct ComputeBudget is to be passed to vm for tracking compute unit consumption, but it's process_instructions(...) are being used elsewhere, unrelate to vm, to parse and get limits compute_budget_instructions set.

the instruction processing function can/should be separated from the struct that holds budget limits. This would clean up logic, is a precursor step for RuntimeTransaction at #33471

Summary of Changes

  • split instructions processing function into its own module,
  • restore ComputeBudget to be pure structure that is passed via invoke_context.

Fixes #

@tao-stones tao-stones force-pushed the split_compute_budget_instructions_process_from_struct branch 2 times, most recently from f624bf9 to b4317e7 Compare October 3, 2023 23:54
@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Merging #33513 (297353f) into master (7aa0fae) will increase coverage by 0.0%.
Report is 3 commits behind head on master.
The diff coverage is 93.5%.

@@           Coverage Diff           @@
##           master   #33513   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         806      807    +1     
  Lines      217937   217907   -30     
=======================================
- Hits       178322   178318    -4     
+ Misses      39615    39589   -26     

@tao-stones tao-stones marked this pull request as ready for review October 4, 2023 16:51
Copy link
Contributor Author

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

No functional change, a lot tests updates tho

accounts-db/src/accounts.rs Show resolved Hide resolved
accounts-db/src/accounts.rs Show resolved Hide resolved
accounts-db/src/accounts.rs Show resolved Hide resolved
program-runtime/src/compute_budget.rs Show resolved Hide resolved
program-runtime/src/compute_budget.rs Show resolved Hide resolved
program-runtime/src/compute_budget.rs Show resolved Hide resolved
Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

Left a few comments. I think this is the right direction, we want to separate the initial processing from ComputeBudget so we can do this processing early on and carry the result until we need to construct a ComputeBudget

program-runtime/src/compute_budget_processor.rs Outdated Show resolved Hide resolved
program-runtime/src/compute_budget.rs Outdated Show resolved Hide resolved
@tao-stones tao-stones force-pushed the split_compute_budget_instructions_process_from_struct branch from 8704f16 to 0c10518 Compare October 9, 2023 20:01
@tao-stones tao-stones force-pushed the split_compute_budget_instructions_process_from_struct branch 2 times, most recently from e451ecf to 4cdcb77 Compare October 11, 2023 00:09
@alessandrod alessandrod self-requested a review October 18, 2023 23:17
apfitzge
apfitzge previously approved these changes Oct 19, 2023
Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

Most of the diff comes from moved code and tests being changed.
I'm much happier with this than the current state of things; it will make it significantly easier to create a RuntimeTransaction type that holds additional meta data to avoid unneccessary re-processing.

Copy link
Contributor

@alessandrod alessandrod 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, just a couple of minor nits

program-runtime/src/compute_budget.rs Outdated Show resolved Hide resolved
program-runtime/src/compute_budget_processor.rs Outdated Show resolved Hide resolved
program-runtime/src/compute_budget_processor.rs Outdated Show resolved Hide resolved
runtime/src/transaction_priority_details.rs Outdated Show resolved Hide resolved
@tao-stones tao-stones force-pushed the split_compute_budget_instructions_process_from_struct branch from 4cdcb77 to 297353f Compare October 19, 2023 14:56
@tao-stones tao-stones added the automerge Merge this Pull Request automatically once CI passes label Oct 19, 2023
@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Oct 19, 2023
@mergify
Copy link
Contributor

mergify bot commented Oct 19, 2023

automerge label removed due to a CI failure

@tao-stones tao-stones merged commit c73bebe into solana-labs:master Oct 19, 2023
16 checks passed
steviez added a commit to steviez/solana that referenced this pull request Oct 20, 2023
…olana-labs#33513)"

This reverts commit c73bebe.

This commit was found to be a consensus breaking change for the tip of
master.
@steviez
Copy link
Contributor

steviez commented Oct 20, 2023

Just a heads up that this commit is going to get reverted from master. One of the canaries running tip-of-master diverged from consensus, and I narrowed the divergence down to this commit. See #33784 for more details, especially #33784 (comment)

steviez added a commit that referenced this pull request Oct 20, 2023
#33784)

Revert "Split compute budget instructions process from struct itself (#33513)"

This reverts commit c73bebe. This
was found to be a consensus breaking change.
@tao-stones
Copy link
Contributor Author

Just a heads up that this commit is going to get reverted from master. One of the canaries running tip-of-master diverged from consensus, and I narrowed the divergence down to this commit. See #33784 for more details, especially #33784 (comment)

Thanks for heads up, Looking into the cause ...

@ilya-bobyr
Copy link
Contributor

Do we consider solana_program_runtime part of the public API?
If so, this change is not backward compatible.

@tao-stones
Copy link
Contributor Author

Do we consider solana_program_runtime part of the public API? If so, this change is not backward compatible.

It shouldn't be public api. (https://discord.com/channels/428295358100013066/439194979856809985/1166387183389782167)

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.

5 participants