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

Get rid of with_ext_cost_counter #5719

Open
matklad opened this issue Dec 8, 2021 · 7 comments
Open

Get rid of with_ext_cost_counter #5719

matklad opened this issue Dec 8, 2021 · 7 comments
Assignees
Labels
A-params-estimator Area: runtime params estimator C-housekeeping Category: Refactoring, cleanups, code quality T-contract-runtime Team: issues relevant to the contract runtime team

Comments

@matklad
Copy link
Contributor

matklad commented Dec 8, 2021

We have this bit of logic:

pub fn with_ext_cost_counter(f: impl FnOnce(&mut HashMap<ExtCosts, u64>)) {

What it does is that it records the cost we charge for in a thread local. We use in the param estimator to make some asserts – basically, we run something, and we sanity check that the expected number of host functions was called. Eg, here:

fn_cost(ctx, "read_memory_1Mib_10k", ExtCosts::read_memory_byte, 1024 * 1024 * 10_000)

1024 * 1024 * 10_000 is used to assert that we indeed read 1 meg ten thousand times.

The problem here is that we have a second, better infra for that, ProfileData:

/// Profile of gas consumption.
/// When add new cost, the new cost should also be append to Cost::ALL
#[derive(Clone, PartialEq, Eq, BorshSerialize, BorshDeserialize)]
pub struct ProfileData {
data: DataArray,
}

It computes the breakdown of costs for contracts, and is a stable thing which is available via RPC. So, we should port the estimator to profile data

@matklad matklad added C-housekeeping Category: Refactoring, cleanups, code quality T-contract-runtime Team: issues relevant to the contract runtime team A-params-estimator Area: runtime params estimator labels Dec 8, 2021
@matklad
Copy link
Contributor Author

matklad commented Dec 8, 2021

cc @jakmeier

@stale
Copy link

stale bot commented Mar 8, 2022

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Mar 8, 2022
@jakmeier jakmeier self-assigned this Mar 8, 2022
@stale stale bot removed the S-stale label Mar 8, 2022
@stale
Copy link

stale bot commented Jun 6, 2022

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months.
It will be closed in 7 days if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the S-stale label Jun 6, 2022
@akhi3030 akhi3030 removed the S-stale label Jul 8, 2022
@matklad
Copy link
Contributor Author

matklad commented Jul 28, 2022

cc @akhi3030 you might want to look into this one

@akhi3030 akhi3030 assigned akhi3030 and unassigned jakmeier Sep 5, 2022
@akhi3030
Copy link
Collaborator

akhi3030 commented Sep 7, 2022

@matklad: I spent some time looking into this. An initial question is that with_ext_cost_counter appears to be used in production code as well.

https://github.com/near/nearcore/blob/master/runtime/near-vm-logic/src/logic.rs#L2362 storage_write() calls gas_counter.pay_base() which calls inc_ext_costs_counter() which calls with_ext_cost_counter().

Is the goal of this issue to get rid of uses of with_ext_cost_counter or just in the parameter estimator?

@matklad
Copy link
Contributor Author

matklad commented Sep 7, 2022

@akhi3030 production code uses this function to write this info, but the only reader of this information is the estimator. So, but removing it from the estimator, we'll make it dead, and could remove it from prod code as well.

@akhi3030
Copy link
Collaborator

akhi3030 commented Sep 7, 2022

Got it, thanks, makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-params-estimator Area: runtime params estimator C-housekeeping Category: Refactoring, cleanups, code quality T-contract-runtime Team: issues relevant to the contract runtime team
Projects
None yet
Development

No branches or pull requests

3 participants