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

move metrics usage out of program-runtime #3390

Closed

Conversation

kevinheavey
Copy link

@kevinheavey kevinheavey commented Oct 30, 2024

Problem

solana-program-runtime has an annoying solana-metrics dependency that severely impacts build time and dependency hell. It's particularly annoying for litesvm which depends on solana-program-runtime

Summary of Changes

  • Make a new solana-program-runtime-metrics crate
  • Move LoadProgramMetrics into the new crate
  • Move ProgramCacheEntry::new and ProgramCacheEntry::reload into the new crate as free functions
  • Update all affected usage of program-runtime in this repo to use the new crate where necessary

None of these API changes affect crates that are covered by the backwards compatibility policy. I don't think deprecation is the way to go here as it would require waiting a long time to actually remove the dependencies

Copy link

mergify bot commented Oct 30, 2024

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

Copy link

@buffalojoec buffalojoec left a comment

Choose a reason for hiding this comment

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

I don't think this is the best way to go about removing the metrics dependency from the program-runtime.

This adds unnecessary indirection to the program cache API, calling into free functions in some other crate. Additionally, the intention here is to make a program-runtime metrics crate, but that's not what the crate proposed here does. It has a lot of the same dependencies as the program-runtime and shares ELF-loading responsibilities with the main program-runtime crate. Metrics are still a side job.

I wonder if we can just put the metrics struct, the datapoint submissions, and the argument for &mut LoadProgramMetrics behind a metrics feature flag. This way, LiteSVM can just use program-runtime without the metrics feature.

@kevinheavey
Copy link
Author

Something like this? #3477

@buffalojoec
Copy link

Something like this? #3477

Yes. Does that approach solve your problem in LiteSVM?

@kevinheavey
Copy link
Author

Yes. Does that approach solve your problem in LiteSVM?

Not without #3383

@kevinheavey
Copy link
Author

Closing in favour of #3477

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants