[move-stdlib] Implement bcs::constant_serialized_size<T>(): Option<u64> #14984
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
⏱️ 1h 5m total CI duration on this PR
|
igor-aptos
force-pushed
the
igor/use_native_vector_move_range
branch
from
3bcedb1 to
a5af4dc
Compare
igor-aptos
requested review from
msmouse,
lightmark,
grao1991 and
zekun000
as code owners
igor-aptos
force-pushed
the
igor/constant_serialized_size
branch
from
8a929c9 to
a420b4c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## igor/use_native_vector_move_range #14984 +/- ##
====================================================================
- Coverage 60.1% 57.5% -2.7%
====================================================================
Files 858 858
Lines 211455 211510 +55
====================================================================
- Hits 127237 121651 -5586
- Misses 84218 89859 +5641 ☔ View full report in Codecov by Sentry. |
msmouse
reviewed
Oct 16, 2024
| Ok(fields | ||
| .iter() | ||
| .map(|field| constant_serialized_size(&field.layout)) | ||
| .collect::<Result<Option<Vec<_>>, _>>() |
There was a problem hiding this comment.
Can this potentially allocate too much? Shall we bother to write a loop or try_for_each()?
There was a problem hiding this comment.
this is nice, but I agree - safer to not worry about it :)
igor-aptos
force-pushed
the
igor/use_native_vector_move_range
branch
from
a5af4dc to
9fcf365
Compare
igor-aptos
force-pushed
the
igor/constant_serialized_size
branch
from
a420b4c to
4c0b5bc
Compare
igor-aptos
force-pushed
the
igor/constant_serialized_size
branch
from
4c0b5bc to
58152b4
Compare
igor-aptos
requested review from
a team,
JoshLind,
gregnazario,
banool,
bchocho,
sasha8,
gelash,
ibalajiarun,
runtian-zhou,
danielxiangzl and
areshand
as code owners
This comment has been minimized.
This comment has been minimized.
georgemitenkov
approved these changes
Nov 13, 2024
| @@ -110,6 +110,10 @@ module admin::transaction_context_test { | |||
| ), type_info::type_name<T3>()], | |||
| 13 | |||
| ); | |||
|
|
|||
| assert!(option::some(option::destroy_some(payload_opt)) == transaction_context::entry_function_payload(), 13); | |||
There was a problem hiding this comment.
unrelated, but done while working on this PR - added a test that move correctly returns Option from natives, which this PR also does
| /// If this function returned Some() for some type before - it is guaranteed to continue returning Some() | ||
| /// On the other hand, if function has returned None for some type, | ||
| /// it might change in the future to return Some() instead, if size becomes "known". | ||
| native public(friend) fun constant_serialized_size<MoveValue>(): std::option::Option<u64>; |
There was a problem hiding this comment.
nit: import at the top of the file, here just Option?
| /// Note: | ||
| /// For some types it might not be known they have constant size, and function might return None. | ||
| /// For example, signer appears to have constant size, but it's size might change. | ||
| /// If this function returned Some() for some type before - it is guaranteed to continue returning Some() |
| } | ||
|
|
||
| // enum Singleton { |
There was a problem hiding this comment.
This for Move 2 tests only? You have tested it locally? Or will enable later?
There was a problem hiding this comment.
will enable this once framework is compiled with move 2
| } | ||
| Ok(total) | ||
| }, | ||
| MoveTypeLayout::Struct(MoveStructLayout::WithFields(fields)) |
There was a problem hiding this comment.
WithFields and WithTypes should not occur at runtime, I think we can error. As type_to_type_layout returns the runtime variant
|
Thanks! Feel free to land and address the comments in a separate PR if needed |
ziaptos
previously requested changes
Nov 13, 2024
| /// For example, signer appears to have constant size, but it's size might change. | ||
| /// If this function returned Some() for some type before - it is guaranteed to continue returning Some() | ||
| /// On the other hand, if function has returned None for some type, | ||
| /// it might change in the future to return Some() instead, if size becomes "known". |
| @@ -172,6 +174,112 @@ pub fn serialized_size_allowing_delayed_values( | |||
| }) | |||
| } | |||
|
|
|||
| /// Count number of types constant_serialized_size would visit, used for gas charging. | |||
There was a problem hiding this comment.
My comment was that "Count number of types constant_serialized_size would visit" is incorrect. Mainly because constant_serializeed_size returns early if it encounters non-constant seriazible field:
match cur {
Some(cur_value) => total = total.map(|v| v + cur_value),
None => {
total = None; // <--------------------------------------------------------
break;
},
}
Maybe re-comment to: The upper bound of the number of types constant_serialized_size would visit... or something like that?
Also, given that these two functions here are so specific for the current native implementation, I'd just move them to the same file where the native function is defined (I'd say they belong there a lot more than here!).
igor-aptos
force-pushed
the
igor/constant_serialized_size
branch
from
974ad93 to
f6c8829
Compare
|
addressed comments |
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.
This comment has been minimized.
This comment has been minimized.
|
I've looked at bcs::to_bytes / bcs::serialized_size, and we don't charge gas as we go, but charge base upfront, and rest at the end (so if it goes out of gas work is done). not sure if that's best, but it's simpler and in this case even more reasonable (as cost of actual work is pretty cheap), so I've merged two functions, and charge visited count after computation - but both on success and failure path. This should be clean and clear now. let me know if you have any concerns. I'll proceed to land it in the next hour or two |
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.
This comment has been minimized.
This comment has been minimized.
ziaptos
approved these changes
Nov 14, 2024
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
It is sometimes useful to know if type has constants serialized size, and for example perform some optimization based on it.
How Has This Been Tested?
provided unit tests
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist