Io Gas adjustments #11300
Merged
Io Gas adjustments #11300
Conversation
This file contains 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
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
This was referenced Dec 12, 2023
Merged
Merged
msmouse
force-pushed
the
1206_refactor_pricing
branch
from
6806ce9
to
60bbc60
Compare
msmouse
force-pushed
the
1211-alden-io-gas
branch
from
663bea5
to
754add7
Compare
msmouse
force-pushed
the
1206_refactor_pricing
branch
from
60bbc60
to
4e833e9
Compare
msmouse
force-pushed
the
1211-alden-io-gas
branch
from
754add7
to
bd9eeca
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #11300 +/- ##
=========================================
- Coverage 68.7% 68.6% -0.1%
=========================================
Files 764 764
Lines 176947 176995 +48
=========================================
- Hits 121722 121583 -139
- Misses 55225 55412 +187 ☔ View full report in Codecov by Sentry. |
msmouse
force-pushed
the
1206_refactor_pricing
branch
from
4e833e9
to
aacb739
Compare
msmouse
force-pushed
the
1211-alden-io-gas
branch
2 times, most recently
from
b05127f
to
9cf068a
Compare
msmouse
force-pushed
the
1206_refactor_pricing
branch
from
aacb739
to
de475b6
Compare
msmouse
force-pushed
the
1211-alden-io-gas
branch
from
9cf068a
to
4849504
Compare
igor-aptos
requested changes
Dec 12, 2023
Comment on lines
201
to
205
| if self.feature_version >= 12 { | ||
| size.checked_sub(self.legacy_free_write_bytes_quota) | ||
| .unwrap_or(NumBytes::zero()) |
There was a problem hiding this comment.
shouldn't it be the opposite?
| impl StoragePricingV3 { | ||
| impl IoPricingV3 { | ||
| fn read_size(&self, loaded: NumBytes) -> NumBytes { | ||
| if self.feature_version >= 12 { |
There was a problem hiding this comment.
you have multiple places with if self.feature_version >= 12, what's the logic when you introduce new version (i.e. IoPricingV4) vs having these conditions inside?
There was a problem hiding this comment.
if I added v4 it complains that v3 and v4 returns different opaque types here, although they are both STORAGE_IO_PER_STATE_SLOT_READ * NumArgs + STORAGE_IO_PER_STATE_BYTE_READ * NumBytes only differing in the specific numbers inside of NumArgs and NumBytes:
I got lazy and just updated v3..
cc @vgao1996
There was a problem hiding this comment.
I guess I can do
V3(v3) => Either::Right(Either::Left(v3.calculate_read_gas(bytes_loaded))),
V4(v4) => Either::Right(Either::Right(v4.calculate_read_gas(bytes_loaded)))
?
Do you prefer that?
There was a problem hiding this comment.
since some params are not part of "V4" , I prefer having separate V3 and V4 in this case
| @@ -287,7 +287,17 @@ fn charge_load_cost( | |||
|
|
|||
| match loaded { | |||
| Some(Some(num_bytes)) => { | |||
| context.charge(COMMON_LOAD_BASE_NEW + COMMON_LOAD_PER_BYTE * num_bytes) | |||
| if context.gas_feature_version() >= 12 { | |||
There was a problem hiding this comment.
stupid question, why does this not go through IoPricing utilities, like everything else?
There was a problem hiding this comment.
I was just keeping the structure..
Indeed it'll be nice if we can remove this shit as well:
aptos-core/aptos-move/aptos-vm/src/gas.rs
Lines 63 to 88 in b1f3abe
There was a problem hiding this comment.
can you at least add a TODO?
seems like it is very easy to miss this place when modifying gas schedule.
msmouse
force-pushed
the
1211-alden-io-gas
branch
2 times, most recently
from
ae4b209
to
24bdab5
Compare
msmouse
force-pushed
the
1211-alden-io-gas
branch
from
24bdab5
to
db2aecc
Compare
msmouse
force-pushed
the
1211-alden-io-gas
branch
from
db2aecc
to
9df8c4c
Compare
msmouse
force-pushed
the
1211-alden-io-gas
branch
from
9df8c4c
to
febfa30
Compare
igor-aptos
approved these changes
Dec 13, 2023
| @@ -287,7 +287,17 @@ fn charge_load_cost( | |||
|
|
|||
| match loaded { | |||
| Some(Some(num_bytes)) => { | |||
| context.charge(COMMON_LOAD_BASE_NEW + COMMON_LOAD_PER_BYTE * num_bytes) | |||
| if context.gas_feature_version() >= 12 { | |||
There was a problem hiding this comment.
can you at least add a TODO?
seems like it is very easy to miss this place when modifying gas schedule.
msmouse
force-pushed
the
1211-alden-io-gas
branch
from
febfa30
to
ad48063
Compare
grao1991
approved these changes
Dec 13, 2023
1. read bytes charged in 4k steps 2. remove per write op free quota, while make per byte write cheaper tune write gas numbers
msmouse
force-pushed
the
1211-alden-io-gas
branch
from
ad48063
to
3c0ca5c
Compare
msmouse
requested review from
banool,
gregnazario and
0xmaayan
as code owners
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
❌ Forge suite
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Test Plan