-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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?
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 |
There was a problem hiding this 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)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤷 😄
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