Skip to content
This repository has been archived by the owner on Jul 7, 2021. It is now read-only.

Weights from benchmark machine #56

Merged
merged 10 commits into from
May 31, 2021
Merged

Weights from benchmark machine #56

merged 10 commits into from
May 31, 2021

Conversation

JesseAbram
Copy link
Contributor

These weights were done on the benchmark machine. They will most likely need to be redone before launch so it is tied to #37 but maybe we should leave it open just to make sure we re run it based on the finalized chain

@joepetrowski joepetrowski mentioned this pull request May 17, 2021
7 tasks
Copy link
Contributor

@apopiak apopiak left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

Should be run one more time?

Was there issue having the bechmarking machine hooked up to this repo?

@JesseAbram
Copy link
Contributor Author

Should be run one more time?

Was there issue having the bechmarking machine hooked up to this repo?

Yup we def should right before launch, also I currently have a PR out that will change the runtime #88 that would also require a re run

@JesseAbram JesseAbram requested a review from apopiak May 31, 2021 15:22
Copy link
Contributor

@apopiak apopiak left a comment

Choose a reason for hiding this comment

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

LGTM, just one suspicious change

fn new_session(_r: u32, c: u32, ) -> Weight {
(37_164_000 as Weight)
// Standard Error: 5_000
.saturating_add((5_724_000 as Weight).saturating_mul(c as Weight))
Copy link
Contributor

Choose a reason for hiding this comment

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

that's a quite significant weight change
wanna comment on that?

Copy link
Member

Choose a reason for hiding this comment

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

the logic of this function must have changed (hence the additional read per C below), so dunno what level of explanation you are looking for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also thought this was weird so ended up realizing that the removal logic was slightly changed and I was missing that block, tested this locally, fixed it, running it now on the benchmark machine again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm same results on the benchmark machine (may just be in my head), going to look into this more (gonna have to run the benchmarks again later) for now will merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

@shawntabrizi
I just thought it deserved extra attention and scrutiny 🤷 😄

pallets/collator-selection/src/weights.rs Show resolved Hide resolved
@JesseAbram JesseAbram merged commit 49ca6b5 into master May 31, 2021
@JesseAbram JesseAbram deleted the jesse-weights branch May 31, 2021 16:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants