[move-stdlib] Add vector::move_range native function #14863
Merged
+289
−5
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
igor-aptos
requested review from
davidiw,
movekevin,
wrwg,
zekun000 and
georgemitenkov
as code owners
⏱️ 2h 43m total CI duration on this PR
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## igor/move_mem_swap #14863 +/- ##
====================================================
Coverage 60.1% 60.1%
====================================================
Files 857 858 +1
Lines 211341 211455 +114
====================================================
+ Hits 127147 127233 +86
- Misses 84194 84222 +28 ☔ View full report in Codecov by Sentry. |
igor-aptos
force-pushed
the
igor/vector_utilities
branch
from
2253183 to
0f08236
Compare
igor-aptos
force-pushed
the
igor/native_vector_move_range
branch
2 times, most recently
from
d724063 to
e4540db
Compare
igor-aptos
requested review from
a team,
msmouse,
grao1991,
bchocho,
sasha8,
ibalajiarun,
JoshLind,
gregnazario,
runtian-zhou,
danielxiangzl,
banool and
0xmaayan
as code owners
igor-aptos
changed the base branch from
igor/vector_utilities
to
igor/move_mem_swap
igor-aptos
force-pushed
the
igor/native_vector_move_range
branch
from
b36187b to
5b9f388
Compare
|
updated PR with addressed comments |
ziaptos
suggested changes
Oct 22, 2024
Comment on lines
2486
to
2473
| // potentially unnecessary as native call should've checked the types already | ||
| // (unlike other vector functions that are bytecodes) | ||
| check_elem_layout(type_param, from_c)?; | ||
| check_elem_layout(type_param, to_c)?; |
There was a problem hiding this comment.
That's a good question. It has the same indirect guarantees as the from_self != to_self.
| @@ -81,8 +81,26 @@ impl NativeFunctions { | |||
| where | |||
| I: IntoIterator<Item = (AccountAddress, Identifier, Identifier, NativeFunction)>, | |||
| { | |||
| let vector_bytecode_instruction_methods = HashSet::from([ | |||
| "empty", | |||
There was a problem hiding this comment.
I don't think there's a general reason to disallow these in MoveVM. I'd rather keep the filter in AptosVM. Is there any other reason for wanting to remove it from AptosVM?
igor-aptos
force-pushed
the
igor/move_mem_swap
branch
from
170eca3 to
a99eccd
Compare
igor-aptos
force-pushed
the
igor/native_vector_move_range
branch
from
5b9f388 to
fc3c349
Compare
|
addressed all comments. move the filtering back to AptosVM. added comment for redundant check |
igor-aptos
force-pushed
the
igor/native_vector_move_range
branch
from
fc3c349 to
9e0fd82
Compare
igor-aptos
force-pushed
the
igor/move_mem_swap
branch
from
23c63a3 to
c95646e
Compare
igor-aptos
force-pushed
the
igor/native_vector_move_range
branch
from
9e0fd82 to
db496b3
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| { | ||
| return Err(SafeNativeError::Abort { | ||
| abort_code: EINDEX_OUT_OF_BOUNDS, | ||
| }); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
vgao1996
reviewed
Nov 13, 2024
There was a problem hiding this comment.
The implementation looks correct.
I have a question though: what is the rationale to implement this rather complex move_range operation rather than splitting it up into two smaller ones,
- one that removes a slice from a vector
- one that inserts a slice into another vector
?
| let mut from_v = $from.borrow_mut(); | ||
| let mut to_v = $to.borrow_mut(); | ||
|
|
||
| if removal_position.checked_add(length).map_or(true, |end| end > from_v.len()) |
There was a problem hiding this comment.
Personally I find this condition a bit hard to read -- the info density may be a bit too high. No strong objection though.
There was a problem hiding this comment.
it's basically
removal_position + length > from_v.len()
just with checked_add. Not sure how to write it cleaner. Introducing new variables seems even more confusing to me
| @@ -41,5 +41,8 @@ crate::gas_schedule::macros::define_gas_parameters!( | |||
| [bcs_serialized_size_failure: InternalGas, { RELEASE_V1_18.. => "bcs.serialized_size.failure" }, 3676], | |||
|
|
|||
| [mem_swap_base: InternalGas, { RELEASE_V1_24.. => "mem.swap.base" }, 1500], | |||
|
|
|||
| [vector_move_range_base: InternalGas, { RELEASE_V1_24.. => "vector.move_range.base" }, 4000], | |||
| [vector_move_range_per_index_moved: InternalGasPerArg, { RELEASE_V1_24.. => "vector.move_range.per_index_moved" }, 10], | |||
There was a problem hiding this comment.
Wow this is pretty cheap. I hope you've done some crude benchmarks at least?
There was a problem hiding this comment.
In #14862, more concretely these are the results: https://gist.github.com/igor-aptos/e8d4e21edcbc75dddcb9382d4e077665
but yeah, memcopy is extremely cheap
For "one that removes a slice from a vector", where would it put it? in a new vector? if so - you would then need two memcopy's, instead of one - making it two times more expensive. As far as I can tell:
is a single memcopy, as splice is lazy (happens on consumption / drop) I could create 4 native functions (insert, remove, append, split_off), instead of 1, but that would be less general, and overall seems better to have fewer more powerful native functions, than many specialized ones? |
vgao1996
approved these changes
Nov 13, 2024
igor-aptos
force-pushed
the
igor/native_vector_move_range
branch
from
db496b3 to
43449cb
Compare
igor-aptos
force-pushed
the
igor/native_vector_move_range
branch
from
23770a3 to
ce5f67b
Compare
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
Memcopy (i.e.
ptr::copy_nonoverlappinginside ofVec) is extremely efficient, and using Vec operations that use it directly is significantly faster (orders of magnitude on bigger vectors) than issuing operations in move.Operations on
vectorthat can be speed-up:insert,remove,append,split_off.To keep amount of native functions short, instead of having native for each of those, providing one more general native function:
vector::move_range, which is enough to support all 4 of the above, in addition to other uses.Internally, we shortcircuit a few special cases, for faster speed.
How Has This Been Tested?
Full performance evaluation is in the follow-up PR: #14862
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist