Skip to content
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.

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

Pin rust_eth_kzg to 0.5.1 #6608

Merged
merged 2 commits into from
Nov 26, 2024
Merged

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 <[email protected]>
Date:   Mon Nov 25 15:39:27 2024 +1100

    Pin crate_crypto transitive deps

commit e87210a
Author: Michael Sproul <[email protected]>
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
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