Stop conflating EnergyQuanta and FunctionBudget#4930
Open
coolreader18 wants to merge 2 commits intomasterfrom
Open
Stop conflating EnergyQuanta and FunctionBudget#4930coolreader18 wants to merge 2 commits intomasterfrom
coolreader18 wants to merge 2 commits intomasterfrom
Conversation
| // elsewhere. So, we just pretend that this is `EnergyQuanta` when it's actually | ||
| // a different unit, and it doesn't really matter to the client anyway. | ||
| // TODO(noa): maybe we could just have this be zero, unconditionally? | ||
| energy_quanta_used: EnergyQuanta::new(event.execution_energy_used.get().into()), |
Contributor
There was a problem hiding this comment.
We could take this opportunity to rename energy_quanta_used to avoid accidental errors in the future. That seems like it might be worth it.
Collaborator
Author
There was a problem hiding this comment.
This is the ws_v1 client api struct.
| pub status: EventStatus, | ||
| pub reducer_return_value: Option<Bytes>, | ||
| pub energy_quanta_used: EnergyQuanta, | ||
| pub execution_energy_used: FunctionBudget, |
Contributor
There was a problem hiding this comment.
I would also not call this energy if it's not energy. CPU instruction estimate seems more apt.
Contributor
cloutiertyler
left a comment
cloutiertyler
left a comment
There was a problem hiding this comment.
Left some comments.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Changes
A sort of followup to #4884, an alternative to #4927.
In #3832, we made FunctionBudget a different unit than EnergyQuanta, but there were still many places where we treated them the same and directly converted between them. I got confused by that in #4884, and now this PR properly separates them and corrects the v8 energy calculation.
Expected complexity level and risk
3 - touches energy calculation without reverting the problematic #4884, but I'm confident it's correct this time.
Testing