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.

Sign up for GitHub

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

Jump to bottom

function_call_cost number cannot be explained #3094

Closed
MaksymZavershynskyi opened this issue Aug 6, 2020 · 8 comments
Closed

function_call_cost number cannot be explained #3094

MaksymZavershynskyi opened this issue Aug 6, 2020 · 8 comments
Assignees
@evgenykuzyakov @willemneal @olonho
Labels
A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) C-bug Category: This is a bug

Comments

@MaksymZavershynskyi
Copy link
Contributor

function_call_cost is currently 2319861500000 gas, see: https://github.com/nearprotocol/nearcore/blob/master/neard/res/genesis_config.json#L74

This is the major limiting factor that restricts contract call TPS to 200. Initially we thought it is so large because the fee computation accidentally adds it to it the cost of trie update, Wasm compilation and loading, receipt creation for refunds, etc. However:

  • Cost of compiling 100KiB contract is 216750*100*2^10+35445963 which 100 times cheaper than the function call fee;
  • Cost of creating a receipt is 108059500000 which is 21 times cheaper;
  • Base cost of storage write (including trie update): 64196736000 which is 36 times cheaper.

There are no good explanations of where this cost is coming from. It might be an indication of a bug in params estimator or large sub-optimality in the runtime. We need to investigate this issue, especially since it can be an easy way to bump our TPS.
@MaksymZavershynskyi MaksymZavershynskyi added C-bug Category: This is a bug A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) labels Aug 6, 2020
@MaksymZavershynskyi MaksymZavershynskyi mentioned this issue Aug 7, 2020
Merged
@weekly-digest weekly-digest bot mentioned this issue Aug 7, 2020
Closed
@evgenykuzyakov
Copy link
Collaborator

Right now when a contract code is pulled from the trie without cache we rehash it to verify the hash matches the hash given in the account structure.

In particular these 2 lines:
https://github.com/nearprotocol/nearcore/blob/9ca3117e04fc87b89e69c19916f0ed82b7a8af16/runtime/runtime/src/cache.rs#L25

and

https://github.com/nearprotocol/nearcore/blob/9ca3117e04fc87b89e69c19916f0ed82b7a8af16/core/primitives/src/contract.rs#L10

Cost of hashing 200Kb of data according to our math estimator is 8003126 Gas per byte or 8003126 * 200000 = 1600625200000 or 1.6 Tgas on itself. That's why when a function call has an overhead of 1.6 Tgas just for hashing the contract code if it's run on 200Kb of data.

I suggest remove hashing of the contract code completely, since hash from the account ID + trie merkalization should be enough to enforce the code hash matches the code from the Trie.

Also this assert_eq! is the middle of the node is rather stupid and would panic a node on corruption of the data. We're not guarded against memory corruption or trie value corruption, so data corruption should not be a problem either.

The hash should be only verified during deploy action, which is already the case.

@evgenykuzyakov evgenykuzyakov mentioned this issue Aug 7, 2020
Merged
2 tasks
@evgenykuzyakov
Copy link
Collaborator

Also it might be not hashing alone. Just reading 200Kb from the trie costs 1870335 * 200000 according to our tests 0.37 Tgas (pre safety multiplier).

@evgenykuzyakov
Copy link
Collaborator

I think the best way to measure cost of compilation is to run a function call block with cache on (warmed up) and a block with cache off. The difference is the cost of cold compilation.

@MaksymZavershynskyi
Copy link
Contributor Author

FYI singlepass is JIT, so cache helps a little bit but not much.

@evgenykuzyakov
Copy link
Collaborator

evgenykuzyakov commented Aug 8, 2020 •
edited
Loading

We also cache preparation (e.g. gas injection) + contract code storage read. Need to look further what takes the most amount of instructions for a base function call. Maybe allocating 64Mib of RAM for any contract call is an issue.

https://github.com/nearprotocol/nearcore/blob/b26a0daf1dae70dc30623307c4281007286ce3dd/runtime/near-vm-logic/src/config.rs#L148

EDIT: I'll run a test with 2Mib memory limit instead. I think most Rust contract by default set stack start at 1Mib, so we shouldn't limit to 1Mib. Previous default limit was 33 pages.

@bowenwang1996
Copy link
Collaborator

Maybe allocating 64Mib of RAM for any contract call is an issue.

That's a lot :)

evgenykuzyakov pushed a commit that referenced this issue Aug 9, 2020
Pass code_hash to ContractCode when it's available (#3102)
7ee16b5 
Don't re-hash a contract code after a read if the `code_hash` is already available in the account ID. The only place to rehash is when `apply_genesis_records` is called.  

Ref: #3094

## Test plan:

- [x] CI
- [x] Run fee estimator again.

To compare.
**this PR**
```rust
data_receipt_creation_config: DataReceiptCreationConfig {
            base_cost: Fee {
                send_sir: 1035057529250,
                send_not_sir: 1035057529250,
                execution: 1035057529250,
            },
            cost_per_byte: Fee {
                send_sir: 23636040,
                send_not_sir: 23636040,
                execution: 23636040,
            },
        },
```
vs 
```rust
data_receipt_creation_config: DataReceiptCreationConfig {
                base_cost: Fee {
                    send_sir: 4697339419375,
                    send_not_sir: 4697339419375,
                    execution: 4697339419375,
                },
                cost_per_byte: Fee {
                    send_sir: 59357464,
                    send_not_sir: 59357464,
                    execution: 59357464,
                },
            },
```

**this PR**
```rust
function_call_cost: Fee {
                send_sir: 1185240500000,
                send_not_sir: 1185240500000,
                execution: 1185240500000,
            },
            function_call_cost_per_byte: Fee {
                send_sir: 1158368,
                send_not_sir: 1158368,
                execution: 1158368,
            },
```

vs 

```rust
function_call_cost: Fee {
                    send_sir: 2319861500000,
                    send_not_sir: 2319861500000,
                    execution: 2319861500000,
                },
                function_call_cost_per_byte: Fee {
                    send_sir: 2235934,
                    send_not_sir: 2235934,
                    execution: 2235934,
                },
```
@olonho
Copy link
Contributor

olonho commented Aug 18, 2020 •
edited
Loading

As mentioned in #3041 (comment) we do need proper definition of what meaning we expect from every individual fee, especially actions ones. Definition better be present in form of probes, inserted in proper places of the estimator code. @nearmax @evgenykuzyakov could you please cooperate here?

@bowenwang1996
Copy link
Collaborator

Superseded by #4401

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@evgenykuzyakov evgenykuzyakov

@willemneal willemneal

@olonho olonho

Labels
A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) C-bug Category: This is a bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@evgenykuzyakov @willemneal @olonho @bowenwang1996 @MaksymZavershynskyi