Skip to content

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
Collaborator

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
Collaborator

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