Add aggregator_v2::is_at_least API #13246
Conversation
igor-aptos
requested review from
georgemitenkov,
gelash,
zekun000,
davidiw,
movekevin and
wrwg
as code owners
⏱️ 1h 2m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13246 +/- ##
========================================
Coverage 33.0% 33.1%
========================================
Files 1753 1753
Lines 337797 337871 +74
========================================
+ Hits 111802 111928 +126
+ Misses 225995 225943 -52 ☔ View full report in Codecov by Sentry. |
|
|
||
| debug_assert_eq!(args.len(), 2); | ||
| debug_assert_eq!(ty_args.len(), 1); | ||
| context.charge(AGGREGATOR_V2_TRY_SUB_BASE)?; |
There was a problem hiding this comment.
This is re-used because the cost is the same as sub?
| @@ -68,7 +79,9 @@ impl DelayedFieldData { | |||
| DelayedChange::Create(DelayedFieldValue::Aggregator(value)) => { | |||
| match math.unsigned_add_delta(*value, &input) { | |||
| Ok(new_value) => { | |||
| *value = new_value; | |||
| if apply_delta { | |||
There was a problem hiding this comment.
nit: Ok(new_value) if apply_delta => {}, _ => {}?
There was a problem hiding this comment.
return is different?
igor-aptos
force-pushed
the
igor/is_at_least_api
branch
from
614d4a9
to
dede930
Compare
igor-aptos
force-pushed
the
igor/is_at_least_api
branch
from
dede930
to
e5de474
Compare
| /// which has no parallelism impact. | ||
| /// If called in a transaction that also modifies the aggregator, or has other read/write conflicts, | ||
| /// it will sequentialize that transaction. (i.e. up to concurrency_level times slower) | ||
| /// If called in a separate transaction (i.e. after transaction that modifies aggregator), it might be |
There was a problem hiding this comment.
is this based on measurements, or what's the reasoning here?
| @@ -55,7 +66,7 @@ impl DelayedFieldData { | |||
| &input, | |||
| max_value, | |||
| )?; | |||
| if result { | |||
| if result && apply_delta { | |||
There was a problem hiding this comment.
I assume apply_delta = false is the new case, in which case is the behavior supposed to mirror apply_delta = true but without modifying self? Can w add one-liner comment?
I think apply_delta can and should be statically known parameter: every call is actually statically set true or false, and that way, with generics or macros we should be able to force self to be &mut or & based on the apply_delta value. Then the compiler will prove that when apply_delta = false, there can be no side effects, and it won't be error prone in the future to refactor this function and accidentally forget to guard mutations with apply_delta. It will also avoid the need of tests on apply_delta.
Of course we can also have separate methods, but that'd require duplication significant code.
There was a problem hiding this comment.
synced up to try and do this in followup, if reasonably readable
| add<Element>(addr, use_type, i, b); | ||
| } | ||
|
|
||
| // issue with type inference of lambda or downcasting from mut to non-mut? |
There was a problem hiding this comment.
lambda doesn't seem to work when first instruction infers &T, and second infers &mut T, it doesn't compile
| max_value, | ||
| SignedU128::Negative(rhs), | ||
| resolver, | ||
| false, |
There was a problem hiding this comment.
if this is the only use of 'false', and since here we only need to check if the value of the aggregator is at least rhs, wouldn't a simpler implementation that avoids modifying try_add_delta be to read lhs, and then share the code below with BoundedMath.unsigned_subtract? (the computation of lhs would be conditional - and that can be isolated and tested separately).
There was a problem hiding this comment.
we need to call resolver.delayed_field_try_add_delta_outcome to capture the restriction
| @@ -270,6 +282,42 @@ fn native_try_sub( | |||
| Ok(smallvec![Value::bool(success)]) | |||
| } | |||
|
|
|||
| fn native_is_at_least_impl( | |||
There was a problem hiding this comment.
does this (and can it, if not) have a simple unit test?
There was a problem hiding this comment.
there is unit tests on the delayed_field_extension.rs, and there are move e2e tests for native calls.
igor-aptos
force-pushed
the
igor/optimize_fa_apt
branch
3 times, most recently
from
b2a904d
to
4a19bb1
Compare
igor-aptos
force-pushed
the
igor/is_at_least_api
branch
from
e5de474
to
f2a3e47
Compare
igor-aptos
force-pushed
the
igor/is_at_least_api
branch
from
f2a3e47
to
121427d
Compare
igor-aptos
force-pushed
the
igor/is_at_least_api
branch
2 times, most recently
from
5e4724f
to
386060c
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.
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.
igor-aptos
force-pushed
the
igor/is_at_least_api
branch
from
386060c
to
9f49c9c
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
|
We can have an efficient aggregator_v2::is_at_least call (and is cheaper than equivalent try_sub() + add() calls), which can then be used to check for minimal balance, or other things.
we don't need really is_at_most(value), as that is really equivalent to !is_at_least(value + 1).
Adding also constructors with value, for now just as move utility, we can see if we want to make them a single native call (instead of currently two). but creation of aggregators is much less frequent, so should be fine.
Description
Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Key Areas to Review
Checklist