Skip to content

A0-4022: Reduced multi block contract migration weight by 4 #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Marcin-Radecki merged 2 commits into from
Feb 20, 2024
Merged

A0-4022: Reduced multi block contract migration weight by 4 #2

Marcin-Radecki merged 2 commits into from
Feb 20, 2024

Conversation

@Marcin-Radecki
Copy link

@Marcin-Radecki Marcin-Radecki commented Feb 16, 2024 •
edited
Loading

@Marcin-Radecki Marcin-Radecki marked this pull request as ready for review February 16, 2024 13:50
pmikolajczyk41
pmikolajczyk41 approved these changes Feb 19, 2024
View reviewed changes
Copy link
Member

@pmikolajczyk41 pmikolajczyk41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks okay, but even with saturation, x/4 != x in 75% of cases :P

substrate/frame/contracts/src/lib.rs
remaining_weight.saturating_reduce(weight);
const reduced_weight_factor: u64 = 4;
let (result, weight) = Migration::<T>::migrate(remaining_weight.saturating_div(reduced_weight_factor));
remaining_weight.saturating_reduce(weight.saturating_mul(reduced_weight_factor));
Copy link
Member

@pmikolajczyk41 pmikolajczyk41 Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this saturating_reduce is weird xD it requires One and it's suspicious that it is more effective that saturating_sub (because that's my guess for the only reason for such a function)

Copy link

@DamianStraszak DamianStraszak Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's actually less effective, it's only that saturating_sub outputs a value and saturating_reduce mutates self. But why the hell is saturating_reduce implemented the way it is I have no idea

substrate/frame/contracts/src/lib.rs
remaining_weight.saturating_reduce(weight.saturating_mul(reduced_weight_factor));

match result {
// There is not enough weight to perform a migration, or make any progress, we
Copy link
Member

@pmikolajczyk41 pmikolajczyk41 Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is actually not correct to return from on_idle in the first case of the match expression - we should try making some other work (like process_deletion_queue) - does this sound reasonable?

Copy link

@DamianStraszak DamianStraszak Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are probably right, we could break instead, but it doesn't matter much anyway, as in practice the migration take just a few blocks of time

@Marcin-Radecki Marcin-Radecki merged commit 180a545 into aleph-v1.2.0 Feb 20, 2024
lesniak43 pushed a commit that referenced this pull request Mar 11, 2024
@Marcin-Radecki @lesniak43
A0-4022: Reduced multi block contract migration weight by 4 (#2)
6ccdf5e 
* A0-4022: Reduced multi block contract migration weight by 4

* Previous approach was a no-op
lesniak43 pushed a commit that referenced this pull request Mar 19, 2024
@AndreiEres @sandreim
subsystem-bench: Prepare CI output (paritytech#3158)
9e6298e 
1. Benchmark results are collected in a single struct.
2. The output of the results is prettified.
3. The result struct used to save the output as a yaml and store it in
artifacts in a CI job.

```
$ cargo run -p polkadot-subsystem-bench --release -- test-sequence --path polkadot/node/subsystem-bench/examples/availability_read.yaml | tee output.txt
$ cat output.txt

polkadot/node/subsystem-bench/examples/availability_read.yaml #1

Network usage, KiB                     total   per block
Received from peers               510796.000  170265.333
Sent to peers                        221.000      73.667

CPU usage, s                           total   per block
availability-recovery                 38.671      12.890
Test environment                       0.255       0.085


polkadot/node/subsystem-bench/examples/availability_read.yaml #2

Network usage, KiB                     total   per block
Received from peers               413633.000  137877.667
Sent to peers                        353.000     117.667

CPU usage, s                           total   per block
availability-recovery                 52.630      17.543
Test environment                       0.271       0.090


polkadot/node/subsystem-bench/examples/availability_read.yaml #3

Network usage, KiB                     total   per block
Received from peers               424379.000  141459.667
Sent to peers                        703.000     234.333

CPU usage, s                           total   per block
availability-recovery                 51.128      17.043
Test environment                       0.502       0.167

```

```
$ cargo run -p polkadot-subsystem-bench --release -- --ci test-sequence --path polkadot/node/subsystem-bench/examples/availability_read.yaml | tee output.txt
$ cat output.txt
- benchmark_name: 'polkadot/node/subsystem-bench/examples/availability_read.yaml #1'
  network:
  - resource: Received from peers
    total: 509011.0
    per_block: 169670.33333333334
  - resource: Sent to peers
    total: 220.0
    per_block: 73.33333333333333
  cpu:
  - resource: availability-recovery
    total: 31.845848445
    per_block: 10.615282815
  - resource: Test environment
    total: 0.23582828799999941
    per_block: 0.07860942933333313

- benchmark_name: 'polkadot/node/subsystem-bench/examples/availability_read.yaml #2'
  network:
  - resource: Received from peers
    total: 411738.0
    per_block: 137246.0
  - resource: Sent to peers
    total: 351.0
    per_block: 117.0
  cpu:
  - resource: availability-recovery
    total: 18.93596025099999
    per_block: 6.31198675033333
  - resource: Test environment
    total: 0.2541994199999979
    per_block: 0.0847331399999993

- benchmark_name: 'polkadot/node/subsystem-bench/examples/availability_read.yaml #3'
  network:
  - resource: Received from peers
    total: 424548.0
    per_block: 141516.0
  - resource: Sent to peers
    total: 703.0
    per_block: 234.33333333333334
  cpu:
  - resource: availability-recovery
    total: 16.54178526900001
    per_block: 5.513928423000003
  - resource: Test environment
    total: 0.43960946299999537
    per_block: 0.14653648766666513
```

---------

Co-authored-by: Andrei Sandu <[email protected]>
lesniak43 pushed a commit that referenced this pull request Apr 8, 2024
@Marcin-Radecki @lesniak43
A0-4022: Reduced multi block contract migration weight by 4 (#2)
419570c 
* A0-4022: Reduced multi block contract migration weight by 4

* Previous approach was a no-op
lesniak43 pushed a commit that referenced this pull request Apr 22, 2024
@Marcin-Radecki @lesniak43
A0-4022: Reduced multi block contract migration weight by 4 (#2)
a552592 
* A0-4022: Reduced multi block contract migration weight by 4

* Previous approach was a no-op
lesniak43 pushed a commit that referenced this pull request May 24, 2024
@Marcin-Radecki @lesniak43
A0-4022: Reduced multi block contract migration weight by 4 (#2)
a213e2a 
* A0-4022: Reduced multi block contract migration weight by 4

* Previous approach was a no-op
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DamianStraszak DamianStraszak DamianStraszak approved these changes

@pmikolajczyk41 pmikolajczyk41 pmikolajczyk41 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Marcin-Radecki @DamianStraszak @pmikolajczyk41