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

Parameter weights tracking issue #8032

Closed
6 tasks done
Tracked by #8550
jakmeier opened this issue Nov 10, 2022 · 10 comments
Closed
6 tasks done
Tracked by #8550

Parameter weights tracking issue #8032

jakmeier opened this issue Nov 10, 2022 · 10 comments
Assignees
Labels
A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) C-tracking-issue Category: a tracking issue T-contract-runtime Team: issues relevant to the contract runtime team

Comments

@jakmeier
Copy link
Contributor

jakmeier commented Nov 10, 2022

The motivation and the high-level idea of parameters weights are outline on gov.near.org.

Implementation work

  1. A-transaction-runtime T-contract-runtime T-core
    jakmeier
  2. A-params-estimator A-transaction-runtime T-contract-runtime
    akashin
  3. A-transaction-runtime T-contract-runtime
    akashin
  4. A-transaction-runtime T-contract-runtime
    akashin
  5. A-contract-runtime
    akashin
  6. A-contract-runtime S-automerge
    akashin

Design work

  • Q: How does a gas weight affect the gas price changes for full blocks?
  • Write a NEP

Related work

Improve gas profiles; #8261

History and Comparison to Other Ideas

Previously, this proposal was named gas weights, now renamed to parameter weights to avoid name clashing with GasWeight to split remaining attached gas relatively between cross contract calls.

The idea for the split between what values we charge and what values we use to fill chunks is from @Ekleog who wrote the details down in a proposal for background CPU tasks using different gas than the main thread: #7625

Here I've stolen the idea and applied it to each parameter separately. The purpose is a bit different now, we just want to specifically combat the situation where we have an undercharging that cannot be solved immediately. The other idea, which would split gas costs into different types of gas, could also help to make execution more efficient overall. For example, it allows using 100% of background CPU and 100% of the main thread, whereas today we have to stop filling a chunk as soon as the sum of gas reaches the limit, regardless on what it was spent on.

@jakmeier jakmeier added A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) C-tracking-issue Category: a tracking issue labels Nov 10, 2022
@jakmeier jakmeier self-assigned this Nov 10, 2022
@jakmeier jakmeier changed the title Gas weights tracking issue Parameter weights tracking issue Nov 11, 2022
@bowenwang1996
Copy link
Collaborator

bowenwang1996 commented Dec 19, 2022

@jakmeier instead of attaching explicit weights to gas costs, it seems that we could achieve the same result with less granularity if we could have "weight" for gas limit of a shard

@jakmeier
Copy link
Contributor Author

jakmeier commented Dec 19, 2022

@bowenwang1996 I feel like I'm not quite understanding your suggestion? If by "less granularity" you meant that we just throttle the entire chain regardless of what the gas is used on, then yes. In fact, we could have almost the same thing by just reducing the gas limit without introducing the concept of a weight. But the point here is to have the granularity so that we can temporarily fix, let's say a 3x undercharging issue without throttling throughput by 3x. That's why I want the weight to be per parameter instead of global.

@bowenwang1996
Copy link
Collaborator

@jakmeier yeah what you said make sense, though I am not sure whether we need the ability to adjust weight for a specific cost if we can dynamically adjust the gas limit through on-chain voting. The idea is that when some undercharging is triggered, validators can vote to lower the gas limit quickly and revert back when there is no more undercharging observed. If we have some logic implemented in nearcore to allow for easy on-chain voting (just like for new protocol versions), then theoretically we can adjust the gas limit for a given shard (or globally on every shard) very quickly. Also, likely we want to figure out how to form this kind of consensus anyways even if we go with parameter weights because we would like to have the ability to quickly adjust the weights without having to go through a new release.

@jakmeier
Copy link
Contributor Author

Oh I see, yes for the dynamic abilities we might not need the granularity.

Since I couldn't find much support for dynamic on-chain voting inside the protocol team, I am thinking of parameter weights as static feature, that must change only with protocol upgrades. Dynamic changes of any kind, granular or not, would be a new endeavor. I'll keep it in mind but no active work is going in that direction for now.

@jakmeier jakmeier added the T-contract-runtime Team: issues relevant to the contract runtime team label Dec 21, 2022
@bowenwang1996
Copy link
Collaborator

Since I couldn't find much support for dynamic on-chain voting inside the protocol team

What is the reason for not supporting it?

@jakmeier
Copy link
Contributor Author

I would summarize it as people mostly think it's too complicated and unwarranted to introduce new instances of on-chain voting for this. Let me elaborate below.

Agreeing on dynamically adjusted weights is tricky to implement, especially if we want it to be lower latency than the current protocol upgrade voting. If the latency is the same, it's unclear what the benefit would be over just making a protocol upgrade and adjusting parameters manually.

Furthermore, adjusting the values automagically can be scary. The same mechanism that is supposed to self-heal the chain when it gets slow could also cripple users, due to throttled throughput. And the control problem(s) we need to solve are not trivial. (Slow execution depends on the workload, the workloads are bursty and inconsistent, and isolating workload per parameters is hard in our code base.) Overall, we might end up doing more harm than good.

All this critique is fair when considering dynamic parameter weights. Before we consider making it dynamic, we should solve the static case and observe it for some time. (If we implement #8258 we can observe gas vs compute time in real-time on a Grafana board.)

But speaking more generally, we could also pursue implementing a dynamic gas/chunk limit which has nothing to do with parameters. This would be simpler to implement. But it's just a different feature that solves different problems. Parameter weights are designed to temporarily resolve known undercharging issues in a very targeted way. Dynamic gas limits would be a very broad solution to resolve execution slowness of an unknown source.

I think parameter weights are more effective in solving the problems we face today, as we can target just the areas we know are problematic. However, if you think gas/chunk limits would be something we should work on in the short-term, I can create a separate issue for that as well if you want.

@Longarithm Longarithm mentioned this issue Dec 23, 2022
26 tasks
@Longarithm
Copy link
Member

Does it make sense to have parameter weight > 1 for writing storage keys?
The reason is that flat storage introduces additional in-memory load based on summary storage keys length written to storage. If we have a weight there, size could be more nicely limited, given that people rarely use long keys anyway: #8436. This is likely to be not needed, but I want to check if this is an option.

@jakmeier
Copy link
Contributor Author

If that is required to ship a first version of flat storage, yes, we could do that as a temporary solution. I could imagine this would be completely unnoticeable in practice anyway.

Still, I don't think this would be a great final solution. Perhaps just increasing the cost should be the long term goal.

@jakmeier jakmeier removed their assignment Feb 2, 2023
@jakmeier
Copy link
Contributor Author

jakmeier commented Feb 2, 2023

Just came to mind: We should also define how view calls are affected, they have a separate gas limit introduced in #4381. It's not relevant for the NEP, as it doesn't affect the protocol specification. But we need to think about how we will implement it.

@aborg-dev aborg-dev self-assigned this Mar 23, 2023
near-bulldozer bot pushed a commit that referenced this issue Mar 24, 2023
This seemed like the simplest approach that introduced minimal runtime overhead and code changes (as opposed to introducing a dedicated field in the GasCounter). Happy to consider the alternatives though.

Part of #8032

Some follow-up work is planned in #8795
nikurt pushed a commit to nikurt/nearcore that referenced this issue Apr 5, 2023
This seemed like the simplest approach that introduced minimal runtime overhead and code changes (as opposed to introducing a dedicated field in the GasCounter). Happy to consider the alternatives though.

Part of near#8032

Some follow-up work is planned in near#8795
@aborg-dev
Copy link
Contributor

Compute Costs have been submitted and stabilized, so I declare this issue resolved. For any future applications of compute costs, we should file new issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-transaction-runtime Area: transaction runtime (transaction and receipts processing, state transition, etc) C-tracking-issue Category: a tracking issue T-contract-runtime Team: issues relevant to the contract runtime team
Projects
None yet
Development

No branches or pull requests

4 participants