Mark libserialize functions as inline by BurntPizza · Pull Request #53393 · rust-lang/rust

BurntPizza · 2018-08-15T15:55:44Z

Got to thinking: "what if that big pile of tiny functions isn't inlining as it should?"
So a few replace-regex later the local perf run says this:

Details

Not huge, but still a win, which is interesting. Want to verify with the real perf run, but I understand there's a backlog.

I didn't notice any increase in compile time or binary sizes for rustc/libs.

rust-highfive · 2018-08-15T15:55:54Z

r? @dtolnay

(rust_highfive has picked a reviewer for you, use r? to override)

dtolnay · 2018-08-15T16:19:05Z

Nice find. Looks good to me if the benchmarks are corroborated.

@bors try
@Mark-Simulacrum @kennytm

bors · 2018-08-15T16:19:17Z

⌛ Trying commit e59ea2fc56ef72b3ac332ccd28310d0662a186c0 with merge e895c391336eabdd2ec4d9f4c94a45c94f6902b0...

michaelwoerister · 2018-08-15T16:21:20Z

Clearly this isn't my fault, as nobody made a lint for it!

michaelwoerister · 2018-08-15T16:21:48Z

BurntPizza · 2018-08-15T16:39:07Z

Rebased.

Some people, when confronted with a problem, think "I know, I'll use regular expressions." Now they have two #[inline]s.

BurntPizza · 2018-08-16T19:52:39Z

@Mark-Simulacrum Could I get a perf run? Or are we waiting on making a dent in the queue?

scottmcm · 2018-08-16T19:56:44Z

 impl<
 T: Encodable
 > Encodable for LinkedList<T> {
+ #[inline]


I thought #[inline] was unnecessary on generic methods?

IIRC, ever since multiple codegen units, it can matter. I forget.

Here's the discussion I remembered: #52704

scottmcm · 2018-08-16T19:57:43Z

 }

+ #[inline]
 pub fn read_raw_bytes(&mut self, s: &mut [u8]) -> Result<(), String> {


The #[inline]s on things like this and write_signed_leb128 seem definitely good, though 👍

Mark-Simulacrum · 2018-08-16T21:56:08Z

@rust-timer build e895c391336eabdd2ec4d9f4c94a45c94f6902b0

rust-timer · 2018-08-16T21:56:09Z

Success: Queued e895c391336eabdd2ec4d9f4c94a45c94f6902b0 with parent 5db71db, comparison URL.

BurntPizza · 2018-08-17T02:01:34Z

Looks like it's ready for r+

dtolnay · 2018-08-17T03:32:02Z

Thanks @scottmcm, I hadn't seen that thread. @alexcrichton can you tell whether any of the downsides of #[inline] would apply to these functions in libserialize? These are only intended to be called by the compiler (unlike functions in the standard library) and the perf comparison looks very good to me.

r? @alexcrichton

BurntPizza · 2018-08-17T03:47:01Z

@dtolnay
There's probably a subset of the #[inline]s that were added that are responsible for the perf increase, and there may be a subset that is detrimental, but finding out what they are would be quite tedious, so that's why I just marked everything and tried it.

alexcrichton · 2018-08-17T06:20:45Z

Ah yeah I explained in a bit more detail here, but none of the #[inline] on generics should be necessary but the tags pointed out by @scottmcm are likely responsible for the speedup! Could those be left and the others removed?

dtolnay · 2018-08-17T06:23:33Z

Thanks, we'll remove all the ones on generic functions.

r? @dtolnay

BurntPizza · 2018-08-17T07:01:31Z

Done. I'd like to check perf again, just to be sure.

kennytm · 2018-08-17T07:12:41Z

@bors try

bors · 2018-08-17T07:12:52Z

⌛ Trying commit 1540e8c with merge 4c76652fddb127b9ed67aa60a665ab1d37f0e97f...

bors · 2018-08-17T09:09:28Z

☀️ Test successful - status-travis
State: approved= try=True

kennytm · 2018-08-17T09:35:39Z

@rust-timer build 4c76652fddb127b9ed67aa60a665ab1d37f0e97f

rust-timer · 2018-08-17T09:35:40Z

Success: Queued 4c76652fddb127b9ed67aa60a665ab1d37f0e97f with parent f34933b, comparison URL.

BurntPizza · 2018-08-17T14:11:26Z

Ah-ha! Interesting. Looks too consistently worse to e noise.
cc @alexcrichton What do you make of this? Did I remove some inlines that weren't generic? Maybe there's a interaction with specialization (some of the inlines removed were on specialized functions)?
EDIT: or LLVM just actually needs the hints here.

alexcrichton · 2018-08-20T16:32:31Z

@bors: r+

Ok I'm gonna go ahead and approve this because the functions tagged for inlining here are "obvious candidates for inlining". While it's true that not all the performance was recovered from inlining everything I think this is a good baseline to start from. I would personally prefer further analysis to figure out which functions actually need to be inlined instead of inlining all of them, which may be possible through diffing perf runs with everything inlined and without everything inlined (hottest functions at the top are likely the candidates to be inlined).

I suspect you're right in that LLVM just actually needs an extra hint for some of the functions here to inline them.

bors · 2018-08-20T16:32:32Z

📌 Commit 1540e8c has been approved by alexcrichton

…xcrichton Mark libserialize functions as inline Got to thinking: "what if that big pile of tiny functions isn't inlining as it should?" So a few `replace-regex` later the local perf run says this: <details> ![](https://i.imgur.com/gvdJEgG.png) </details> Not huge, but still a win, which is interesting. Want to verify with the real perf run, but I understand there's a backlog. I didn't notice any increase in compile time or binary sizes for rustc/libs.

@ghost

Rollup of 17 pull requests Successful merges: - #53030 (Updated RELEASES.md for 1.29.0) - #53104 (expand the documentation on the `Unpin` trait) - #53213 (Stabilize IP associated constants) - #53296 (When closure with no arguments was expected, suggest wrapping) - #53329 (Replace usages of ptr::offset with ptr::{add,sub}.) - #53363 (add individual docs to `core::num::NonZero*`) - #53370 (Stabilize macro_vis_matcher) - #53393 (Mark libserialize functions as inline) - #53405 (restore the page title after escaping out of a search) - #53452 (Change target triple used to check for lldb in build-manifest) - #53462 (Document Box::into_raw returns non-null ptr) - #53465 (Remove LinkMeta struct) - #53492 (update lld submodule to include RISCV patch) - #53496 (Fix typos found by codespell.) - #53521 (syntax: Optimize some literal parsing) - #53540 (Moved issue-53157.rs into src/test/ui/consts/const-eval/) - #53551 (Avoid some Place clones.) Failed merges: r? @ghost

bors · 2018-08-21T18:16:39Z

⌛ Testing commit 1540e8c with merge 9f9f2c0...

kennytm · 2018-08-24T04:15:23Z

@bors retry r-

rust-highfive assigned dtolnay Aug 15, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2018

michaelwoerister reviewed Aug 15, 2018
View reviewed changes

Comment threadsrc/libserialize/serialize.rs Outdated
michaelwoeristerAug 15, 2018
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:O
mattico reacted with laugh emoji

Mark libserialize functions as inline
bc900f5

BurntPizza force-pushed the serialize-inlines branch from e59ea2f to bc900f5Compare August 15, 2018 16:37

scottmcm reviewed Aug 16, 2018
View reviewed changes

rust-highfive assigned alexcrichton and unassigned dtolnay Aug 17, 2018

rust-highfive assigned dtolnay and unassigned alexcrichton Aug 17, 2018

Remove inline attribute on generic functions
1540e8c

kennytm added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 17, 2018

kennytm removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 17, 2018

dtolnay assigned alexcrichton and unassigned dtolnay Aug 17, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2018

kennytm mentioned this pull request Aug 20, 2018
Rollup of 17 pull requests #53530
Merged

bors merged commit 1540e8c into rust-lang:masterAug 21, 2018

BurntPizza deleted the serialize-inlines branch August 24, 2018 00:51

Uh oh!

Uh oh!

Conversation

BurntPizza commented Aug 15, 2018

Uh oh!

rust-highfive commented Aug 15, 2018

Uh oh!

dtolnay commented Aug 15, 2018

Uh oh!

bors commented Aug 15, 2018

Uh oh!

michaelwoeristerAug 15, 2018

Choose a reason for hiding this comment

Uh oh!

BurntPizzaAug 15, 2018

Choose a reason for hiding this comment

Uh oh!

michaelwoeristerAug 15, 2018

Choose a reason for hiding this comment

Uh oh!

BurntPizza commented Aug 15, 2018

Uh oh!

BurntPizza commented Aug 16, 2018

Uh oh!

scottmcmAug 16, 2018

Choose a reason for hiding this comment

Uh oh!

BurntPizzaAug 16, 2018

Choose a reason for hiding this comment

Uh oh!

scottmcmAug 16, 2018

Choose a reason for hiding this comment

Uh oh!

scottmcmAug 16, 2018

Choose a reason for hiding this comment

Uh oh!

Mark-Simulacrum commented Aug 16, 2018

Uh oh!

rust-timer commented Aug 16, 2018

Uh oh!

BurntPizza commented Aug 17, 2018

Uh oh!

dtolnay commented Aug 17, 2018

Uh oh!

BurntPizza commented Aug 17, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton commented Aug 17, 2018

Uh oh!

dtolnay commented Aug 17, 2018

Uh oh!

BurntPizza commented Aug 17, 2018

Uh oh!

kennytm commented Aug 17, 2018

Uh oh!

bors commented Aug 17, 2018

Uh oh!

bors commented Aug 17, 2018

Uh oh!

kennytm commented Aug 17, 2018

Uh oh!

rust-timer commented Aug 17, 2018

Uh oh!

BurntPizza commented Aug 17, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexcrichton commented Aug 20, 2018

Uh oh!

bors commented Aug 20, 2018

Uh oh!

bors commented Aug 21, 2018

Uh oh!

kennytm commented Aug 24, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

BurntPizza commented Aug 17, 2018•
edited
Loading

BurntPizza commented Aug 17, 2018•
edited
Loading

kennytm commented Aug 24, 2018•
edited
Loading