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 runtime-transaction crate and reduce SVM dependency #3983

Merged
merged 4 commits into from
Dec 10, 2024

Conversation

pgarg66
Copy link

@pgarg66 pgarg66 commented Dec 6, 2024

Problem

SVM dependency on runtime-transaction can be reduced by splitting relevant code in its own crate.

Summary of Changes

Split compute-budget related code to a new crate compute-budget-instruction.

Fixes #

@pgarg66 pgarg66 force-pushed the split-runtime-transaction-crate branch from 1d959af to 620a09e Compare December 6, 2024 22:01
@pgarg66
Copy link
Author

pgarg66 commented Dec 6, 2024

Hey @kevinheavey, do you happen to have the crate name reserved under your username? This PR is getting this error:
https://github.com/anza-xyz/agave/actions/runs/12206772353/job/34056937257?pr=3983#step:7:22

@pgarg66 pgarg66 force-pushed the split-runtime-transaction-crate branch from 620a09e to 3d473fc Compare December 6, 2024 22:31
@kevinheavey
Copy link

kevinheavey commented Dec 6, 2024

Hey @kevinheavey, do you happen to have the crate name reserved under your username? This PR is getting this error: https://github.com/anza-xyz/agave/actions/runs/12206772353/job/34056937257?pr=3983#step:7:22

Yes I do. I've sent an ownership invitation to anza-team now so @yihau can accept and you can use it for this PR

@pgarg66
Copy link
Author

pgarg66 commented Dec 6, 2024

Hey @kevinheavey, do you happen to have the crate name reserved under your username? This PR is getting this error: https://github.com/anza-xyz/agave/actions/runs/12206772353/job/34056937257?pr=3983#step:7:22

Yes I do. I've sent an ownership invitation to anza-team now so @yihau can accept and you can use it for this PR

Thank you!

@pgarg66 pgarg66 marked this pull request as ready for review December 6, 2024 23:35
@pgarg66 pgarg66 requested a review from a team as a code owner December 6, 2024 23:35
Copy link

@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.

Thanks for picking this up @pgarg66! Left a couple last comments about Clone and FrozenABI.

With this split, I can now:

  • Add compute_budget_limits to SVMMessage trait
    • default implementation calls process_compute_budget_instructions
    • override for structs implementing StaticMeta

This will let SVM be slightly more efficient, since we can use cached-metadata to get the compute-budget instructions quickly, instead of re-calculating. Main thing getting in the way of doing this previously was just the code organization - which this PR solves!

LucasSte
LucasSte previously approved these changes Dec 9, 2024
tao-stones
tao-stones previously approved these changes Dec 9, 2024
Copy link

@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.

lgtm - thanks for doing it

apfitzge
apfitzge previously approved these changes Dec 9, 2024
Copy link

@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.

Thanks for cleaning everything up!

@pgarg66 pgarg66 force-pushed the split-runtime-transaction-crate branch from 298c917 to 45cc59a Compare December 9, 2024 19:25
@pgarg66 pgarg66 requested a review from LucasSte December 9, 2024 19:30
@pgarg66 pgarg66 force-pushed the split-runtime-transaction-crate branch from 45cc59a to dcab4f7 Compare December 10, 2024 00:37
@pgarg66 pgarg66 merged commit 395db14 into anza-xyz:master Dec 10, 2024
51 checks passed
@pgarg66 pgarg66 deleted the split-runtime-transaction-crate branch December 10, 2024 14:15
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