CostTracker: Add a getter to expose cost by writable accounts by ebin-mathews · Pull Request #7920 · anza-xyz/agave

ebin-mathews · 2025-09-05T17:11:36Z

Summary
This change adds a new method to the CostTracker that provides a breakdown of compute unit costs for each writable account during block processing.

Problem
Currently, CostTracker aggregates compute unit costs but doesn't expose a detailed breakdown of costs per account. This makes it difficult to analyze resource consumption patterns and identify which accounts are the biggest resource consumers within a block during post-analysis.

Proposed Changes
A new method, get_cost_by_writable_accounts(), has been added to the CostTracker which exposes a reference to the map that holds the detailed breakdown of costs per account.

This change adds a new method to the CostTracker that provides a breakdown of compute unit costs for each writable account during block processing. Problem Currently, CostTracker aggregates compute unit costs but doesn't expose a detailed breakdown of costs per account. This makes it difficult to analyze resource consumption patterns and identify which accounts are the biggest resource consumers within a block. Solution A new method, get_cost_by_writable_accounts(), has been added to the CostTracker to address this issue. This method returns a mapping of each writable account to its specific compute unit costs, providing a granular view of resource usage. Testing ✅ The new method was tested to ensure it correctly tracks and returns per-account costs. ✅ Existing CostTracker functionality remains unchanged and unaffected by this addition. ✅ The change was validated to confirm it correctly captures the cost distribution among writable accounts. Type of Change [ ] Bug fix (non-breaking change which fixes an issue) [x] New feature (non-breaking change which adds functionality) [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) [ ] Documentation update Checklist [x] Code compiles without errors [x] New tests have been added and pass [x] No breaking changes [x] Follows existing code style

tao-stones · 2025-09-05T17:39:20Z

cost-model/src/cost_tracker.rs

    }
+
+    /// Get a reference to the cost by writable accounts map
+    pub fn get_cost_by_writable_accounts(&self) -> &HashMap<Pubkey, u64, ahash::RandomState> {


The concern with this pub fn is others may start use it in validator, then accidentally increase CostTracker locking period. In validator code space, CostTracker is shared among worker threads, therefore is highly contended, especially in high tps environment. Would be great to prevent such accident.

Few options I thought of:

your project implements this functionality locally, so it does not exist in Agave repo;

if it has to be in Agave, create a separate module with name screaming "only use in post-analyze to avoid lock contention", implement this for CostTracker with a dedicated trait (for example, CostTrackerPostAnalysis). So only intended users will import and use it;

could we hide method behind a feature that is disabled by default?

sounds pretty hacky, but a possible solution. Is there an easy way to explain why #1 above not possible?

if logic changes w/ a new feature gate, assume there's an API breakage which is easy to pick up in users of the software. could be logic change, parameter change, or something like SIMD-322

Sorry, not sure if I'm following. my thought was to do this in your project so upstream isn't polluted:

// In your project cost_tracker_post_analysis.rs use solana-cost-model::CostTracker; pub trait CostTrackerPostAnalysis { fn get_cost_by_writable_accounts(&self); } impl CostTrackerPostAnalysis for CostTracker { ... } // in use side use cost_tracker_post_analysis::CostTrackerPostAnalysis; // use CostTrackerPostAnalysis()

future API changes wouldn't impact your project, but if that Hashmap goes away, you will know 😄

impl CostTrackerPostAnalysis for CostTracker { ... }

This won't work for the CostTracker right as the hash map is private.

I've made some updates so that we have a feature gated trait that implements the getter. So, when the cargo crate is published, can use the feature. For use in agave code, this would require the feature + the trait inclusion, so easy to spot

tao-stones · 2025-09-05T17:53:29Z

Also, maybe related, I am running this branch to collect contended accounts details - similar info you are looking for from the HashMap. Maybe can be repurposed for your need (eg instead of report to metrics, do something differently for the same data)?

tao-stones

It looks good to me. @apfitzge do you have opinions?

apfitzge · 2025-09-05T20:27:30Z

cost-model/src/cost_tracker_post_analysis.rs

+use {solana_pubkey::Pubkey, std::collections::HashMap};
+
+/// Trait to help with post-analysis of a given block
+pub trait CostTrackerPostAnalysis {


if we already have the feature, is there a purpose in having the trait?

Requiring manually use trait to access that function is an additional safety for dev.

I think it comes down to maintainability and making sure the get method is not used inadvertently.
The trait is kept behind a feature and the implementation as well.

So, if someone has to use it within agave code, the change would be very noticeable.
This should deter any such usage as it's not part of the vanilla struct and keeps it distinct.

right but it's 2 things to do the same thing, prevent accidental usage. Just choose one I think

Can keep only the trait, if the general recommendation is not to use the feature gate.

reverted the feature.
@tao-stones

i'll also vote of using trait. Ok without feature gate

right but it's 2 things to do the same thing, prevent accidental usage. Just choose one I think

if we actually cared about safety here, we'd use the type system and transform to a type that specifies the new method only after we've left the "hot path" not pray that dev reads a comment in a different file

apfitzge · 2025-09-05T20:28:53Z

we've generally not added features, as it complicates compilation and testing.

apfitzge · 2025-09-05T20:31:58Z

Problem
Currently, CostTracker aggregates compute unit costs but doesn't expose a detailed breakdown of costs per account. This makes it difficult to analyze resource consumption patterns and identify which accounts are the biggest resource consumers within a block during post-analysis.

I guess I'm still not that convinced this is the right approach. If you're doing post-analysis of the block, why use the cost-tracker which is designed around checking if things can fit into the block.
My other concern is that you build off of this, but we may change this drastically in the future anyway as me make other scheduler changes.

I'm not convinced re-using this is the best option for BAM.

This reverts commit b927337.

tao-stones

lgtm

apfitzge

Thanks for responding to feedback quickly!

codecov-commenter · 2025-09-05T23:29:21Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.1%. Comparing base (19421ef) to head (d14414e).
⚠️ Report is 8 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7920   +/-   ##
=======================================
  Coverage    83.1%    83.1%           
=======================================
  Files         810      810           
  Lines      357419   357436   +17     
=======================================
+ Hits       297211   297255   +44     
+ Misses      60208    60181   -27

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mergify · 2025-09-24T16:53:48Z

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

* Summary This change adds a new method to the CostTracker that provides a breakdown of compute unit costs for each writable account during block processing. Problem Currently, CostTracker aggregates compute unit costs but doesn't expose a detailed breakdown of costs per account. This makes it difficult to analyze resource consumption patterns and identify which accounts are the biggest resource consumers within a block. Solution A new method, get_cost_by_writable_accounts(), has been added to the CostTracker to address this issue. This method returns a mapping of each writable account to its specific compute unit costs, providing a granular view of resource usage. Testing ✅ The new method was tested to ensure it correctly tracks and returns per-account costs. ✅ Existing CostTracker functionality remains unchanged and unaffected by this addition. ✅ The change was validated to confirm it correctly captures the cost distribution among writable accounts. Type of Change [ ] Bug fix (non-breaking change which fixes an issue) [x] New feature (non-breaking change which adds functionality) [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) [ ] Documentation update Checklist [x] Code compiles without errors [x] New tests have been added and pass [x] No breaking changes [x] Follows existing code style * Use a trait to expose the heavy method * gate behind a feature * Revert "gate behind a feature" This reverts commit b927337. (cherry picked from commit f4598b1)

…backport of #7920) (#8174) CostTracker: Add a getter to expose cost by writable accounts (#7920) * Summary This change adds a new method to the CostTracker that provides a breakdown of compute unit costs for each writable account during block processing. Problem Currently, CostTracker aggregates compute unit costs but doesn't expose a detailed breakdown of costs per account. This makes it difficult to analyze resource consumption patterns and identify which accounts are the biggest resource consumers within a block. Solution A new method, get_cost_by_writable_accounts(), has been added to the CostTracker to address this issue. This method returns a mapping of each writable account to its specific compute unit costs, providing a granular view of resource usage. Testing ✅ The new method was tested to ensure it correctly tracks and returns per-account costs. ✅ Existing CostTracker functionality remains unchanged and unaffected by this addition. ✅ The change was validated to confirm it correctly captures the cost distribution among writable accounts. Type of Change [ ] Bug fix (non-breaking change which fixes an issue) [x] New feature (non-breaking change which adds functionality) [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) [ ] Documentation update Checklist [x] Code compiles without errors [x] New tests have been added and pass [x] No breaking changes [x] Follows existing code style * Use a trait to expose the heavy method * gate behind a feature * Revert "gate behind a feature" This reverts commit b927337. (cherry picked from commit f4598b1) Co-authored-by: ebin-mathews <144146969+ebin-mathews@users.noreply.github.com>

mergify bot added community need:merge-assist labels Sep 5, 2025

mergify bot requested a review from a team September 5, 2025 17:12

ebin-mathews changed the title ~~CostTracker: Add a getter tp expose cost by writable accounts~~ CostTracker: Add a getter t0 expose cost by writable accounts Sep 5, 2025

ebin-mathews changed the title ~~CostTracker: Add a getter t0 expose cost by writable accounts~~ CostTracker: Add a getter to expose cost by writable accounts Sep 5, 2025

tao-stones reviewed Sep 5, 2025

View reviewed changes

ebin-mathews added 2 commits September 5, 2025 12:56

Use a trait to expose the heavy method

331d369

gate behind a feature

b927337

tao-stones previously approved these changes Sep 5, 2025

View reviewed changes

apfitzge reviewed Sep 5, 2025

View reviewed changes

Revert "gate behind a feature"

d14414e

This reverts commit b927337.

ebin-mathews dismissed tao-stones’s stale review via d14414e September 5, 2025 20:58

tao-stones approved these changes Sep 5, 2025

View reviewed changes

apfitzge approved these changes Sep 5, 2025

View reviewed changes

tao-stones added automerge automerge Merge this Pull Request automatically once CI passes CI Pull Request is ready to enter CI labels Sep 5, 2025

anza-team removed the CI Pull Request is ready to enter CI label Sep 5, 2025

tao-stones merged commit f4598b1 into anza-xyz:master Sep 5, 2025
46 checks passed

tao-stones added the v3.0 label Sep 24, 2025

mergify bot mentioned this pull request Sep 24, 2025

v3.0: CostTracker: Add a getter to expose cost by writable accounts (backport of #7920) #8174

Merged

Conversation

ebin-mathews commented Sep 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tao-stones commented Sep 5, 2025

Uh oh!

tao-stones left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebin-mathews Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apfitzge commented Sep 5, 2025

Uh oh!

apfitzge commented Sep 5, 2025

Uh oh!

tao-stones left a comment

Choose a reason for hiding this comment

Uh oh!

apfitzge left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Sep 5, 2025

Codecov Report

Uh oh!

Uh oh!

mergify bot commented Sep 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ebin-mathews Sep 5, 2025 •

edited

Loading