Skip to content

Conversation

michaelsproul
Copy link
Member

Proposed Changes

Temporarily pin rust_eth_kzg to 0.5.1 as 0.5.2 seems to have regressed some loading performance enough for our CI to start timing out in the simulator tests (32s timeout). See:

@michaelsproul michaelsproul added ready-for-review The code is ready for review v6.0.0 New major release for hierarchical state diffs labels Nov 25, 2024
@michaelsproul michaelsproul mentioned this pull request Nov 25, 2024
@michaelsproul
Copy link
Member Author

Benchmarks for kzg crate for 0.5.2:

Initialize context rust_eth_kzg
                        time:   [3.8753 s 3.8973 s 3.9200 s]

Benchmarking Initialize context c-kzg (4844): Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.3s, or reduce sample count to 90.
Initialize context c-kzg (4844)
                        time:   [53.268 ms 53.289 ms 53.310 ms]
Found 11 outliers among 100 measurements (11.00%)
  6 (6.00%) high mild
  5 (5.00%) high severe

@michaelsproul
Copy link
Member Author

Context initialisation is 33% faster with 0.5.1:

Initialize context rust_eth_kzg
                        time:   [2.6043 s 2.6138 s 2.6226 s]
                        change: [-33.399% -32.932% -32.494%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 30 outliers among 100 measurements (30.00%)
  21 (21.00%) low severe
  1 (1.00%) low mild
  7 (7.00%) high mild
  1 (1.00%) high severe

Benchmarking Initialize context c-kzg (4844): Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 5.4s, or reduce sample count to 90.
Initialize context c-kzg (4844)
                        time:   [55.333 ms 55.380 ms 55.429 ms]
                        change: [+3.8292% +3.9248% +4.0237%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

michaelsproul added a commit that referenced this pull request Nov 25, 2024
Squashed commit of the following:

commit adc86cd
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Mon Nov 25 15:39:27 2024 +1100

    Pin crate_crypto transitive deps

commit e87210a
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Mon Nov 25 15:05:34 2024 +1100

    Pin rust_eth_kzg to 0.5.1
@michaelsproul michaelsproul mentioned this pull request Nov 25, 2024
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

LGTM

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Nov 26, 2024
@michaelsproul
Copy link
Member Author

@mergify queue

Copy link

mergify bot commented Nov 26, 2024

queue

🛑 The pull request has been removed from the queue default

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

mergify bot added a commit that referenced this pull request Nov 26, 2024
@michaelsproul
Copy link
Member Author

@mergify requeue

Copy link

mergify bot commented Nov 26, 2024

requeue

✅ This pull request will be re-embarked automatically

The followup queue command will be automatically executed to re-embark the pull request

Copy link

mergify bot commented Nov 26, 2024

queue

🛑 The pull request has been removed from the queue default

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

mergify bot added a commit that referenced this pull request Nov 26, 2024
@michaelsproul
Copy link
Member Author

@mergify requeue

Copy link

mergify bot commented Nov 26, 2024

requeue

✅ This pull request will be re-embarked automatically

The followup queue command will be automatically executed to re-embark the pull request

Copy link

mergify bot commented Nov 26, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 720f596

mergify bot added a commit that referenced this pull request Nov 26, 2024
@mergify mergify bot merged commit 720f596 into sigp:unstable Nov 26, 2024
29 checks passed
@michaelsproul michaelsproul deleted the pin-rust-eth-kzg branch January 22, 2025 04:49
mergify bot pushed a commit that referenced this pull request Jan 30, 2025
Update cargo dependencies while keeping `rust_eth_kzg` pinned to `0.5.1` due to the regression described in:

- #6608

The changes from that PR were not sufficient to actually pin the dependencies of `rust_eth_kzg`, because the dependencies from the workspace Cargo.toml file were not being used anywhere. To fix this, I've added them as explicit dependencies in `crypto/kzg/Cargo.toml`. With this change, `cargo update` no longer tries to update them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge. v6.0.0 New major release for hierarchical state diffs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants