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

Inline some serialize impls #71208

Closed

Conversation

ecstatic-morse
Copy link
Contributor

Some trivial methods in impls of the Serialize::Encoder trait are not marked #[inline] but are not generic and are (I believe) used cross-crate.

r? @eddyb

Some trivial methods in impls of the `Serialize::Encoder` trait are not
marked `#[inline]` but are not generic and are (I believe) used
cross-crate.
At present, the macro only adds `#[inline]` to the first method.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 16, 2020
@@ -909,10 +909,12 @@ where

macro_rules! encoder_methods {
($($name:ident($ty:ty);)*) => {
#[inline]
$(fn $name(&mut self, value: $ty) -> Result<(), Self::Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha, this one is great!

Copy link
Member

Choose a reason for hiding this comment

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

Oh dear, I just realized what's going on now.

@ecstatic-morse ecstatic-morse changed the title Inline serialize Inline some serialize impls Apr 16, 2020
@ecstatic-morse
Copy link
Contributor Author

Presuming this will ultimately be approved, do we want to just rollup=never this or do a manual perf run now? rollup=never saves one perf run if this does have an impact, and the results from #69281 indicate that it will probably have a small one.

@nnethercote
Copy link
Contributor

I always do a perf run ahead of time for any change that is intended to improve performance, because sometimes things don't work the way you'd expect.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Apr 16, 2020

⌛ Trying commit 99cca04 with merge 52d7e4551d123138e20df7d2fd74e1998ed3e815...

@eddyb
Copy link
Member

eddyb commented Apr 16, 2020

r=me after perf run

@eddyb
Copy link
Member

eddyb commented Apr 16, 2020

Oh and might as well: @bors rollup=never

@bors
Copy link
Contributor

bors commented Apr 17, 2020

☀️ Try build successful - checks-azure
Build commit: 52d7e4551d123138e20df7d2fd74e1998ed3e815 (52d7e4551d123138e20df7d2fd74e1998ed3e815)

@rust-timer
Copy link
Collaborator

Queued 52d7e4551d123138e20df7d2fd74e1998ed3e815 with parent 7f3df57, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 52d7e4551d123138e20df7d2fd74e1998ed3e815, comparison URL.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Apr 17, 2020

This caused both instruction count and task-clock measurements to regress slightly. I might try a run with only emit_unit inlined, since that is what could be affecting #71044, although obviously I'm less confident now. I guess sometimes things don't work out the way you'd expect 🤣.

@eddyb
Copy link
Member

eddyb commented Apr 17, 2020

Looks like a wash to me, tbh. I wonder if none of these actually matter in practice. I recall @nnethercote did some work with encoding/decoding, maybe they found everything hot enough.

@ecstatic-morse
Copy link
Contributor Author

So I think we can close this. I'm gonna try making emit_unit inline as part of #71044 to see if that addresses the slowdown during metadata encoding I'm seeing there.

I think we should do something about the dangling #[inline] though. Maybe just remove it?

@joelpalmer joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 28, 2020
@Dylan-DPC-zz
Copy link

Closing this based on the above comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants