Inline most of the code paths for conversions with boxed slices#49555

nox · 2018-04-01T10:01:55Z

This helps with the specific problem described in #49541, obviously without making any large change to how inlining works in the general case.

Everything involved in the conversions is made #[inline], except for the <Vec<T>>::into_boxed_slice entry point which is made #[inline(always)] after checking that duplicating the function mentioned in the issue prevented its inlining if I only annotate it with
#[inline].

For the record, that function was:

pub fn foo() -> Box<[u8]> {
    vec![0].into_boxed_slice()
}

To help the inliner's job, we also hoist a self.capacity() != self.len check in <Vec<T>>::shrink_to_fit and mark it as #[inline] too.

This helps with the specific problem described in rust-lang#49541, obviously without making any large change to how inlining works in the general case. Everything involved in the conversions is made `#[inline]`, except for the `<Vec<T>>::into_boxed_slice` entry point which is made `#[inline(always)]` after checking that duplicating the function mentioned in the issue prevented its inlining if I only annotate it with `#[inline]`. For the record, that function was: ```rust pub fn foo() -> Box<[u8]> { vec![0].into_boxed_slice() } ``` To help the inliner's job, we also hoist a `self.capacity() != self.len` check in `<Vec<T>>::shrink_to_fit` and mark it as `#[inline]` too.

eddyb · 2018-04-01T10:10:05Z

r? @alexcrichton

alexcrichton · 2018-04-02T16:14:39Z

src/liballoc/vec.rs

-        self.buf.shrink_to_fit(self.len);
+        if self.capacity() != self.len {
+            self.buf.shrink_to_fit(self.len);
+        }


I think that these don't need the #[inline] attribute because they're already generic, right?

I'll check and report back.

Indeed, this wasn't necessary. I removed it and compiled a module with foo duplicated, here is the result of compiling that to assembly:

.section __TEXT,__text,regular,pure_instructions .p2align 4, 0x90 __ZN55_$LT$alloc..heap..Heap$u20$as$u20$core..heap..Alloc$GT$3oom17h2c0e1b8273a8e186E: .cfi_startproc pushq %rbp .cfi_def_cfa_offset 16 .cfi_offset %rbp, -16 movq %rsp, %rbp .cfi_def_cfa_register %rbp callq ___rust_oom ud2 .cfi_endproc .p2align 4, 0x90 __ZN5alloc4heap15exchange_malloc28_$u7b$$u7b$closure$u7d$$u7d$17h1d897acb095d410dE: .cfi_startproc pushq %rbp .cfi_def_cfa_offset 16 .cfi_offset %rbp, -16 movq %rsp, %rbp .cfi_def_cfa_register %rbp subq $32, %rsp movq 16(%rdi), %rax movq %rax, -8(%rbp) movq (%rdi), %rax movq 8(%rdi), %rcx movq %rcx, -16(%rbp) movq %rax, -24(%rbp) leaq -24(%rbp), %rdi callq __ZN55_$LT$alloc..heap..Heap$u20$as$u20$core..heap..Alloc$GT$3oom17h2c0e1b8273a8e186E ud2 .cfi_endproc .globl _foo .p2align 4, 0x90 _foo: .cfi_startproc pushq %rbp .cfi_def_cfa_offset 16 .cfi_offset %rbp, -16 movq %rsp, %rbp .cfi_def_cfa_register %rbp subq $48, %rsp leaq -40(%rbp), %rdx movl $1, %edi movl $1, %esi callq ___rust_alloc testq %rax, %rax je LBB2_1 movb $0, (%rax) movl $1, %edx addq $48, %rsp popq %rbp retq LBB2_1: movq -32(%rbp), %rax movq -24(%rbp), %rcx movq %rcx, -8(%rbp) movq %rax, -16(%rbp) movq -16(%rbp), %rax movq -8(%rbp), %rcx movq %rcx, -24(%rbp) movq %rax, -32(%rbp) leaq -40(%rbp), %rdi callq __ZN5alloc4heap15exchange_malloc28_$u7b$$u7b$closure$u7d$$u7d$17h1d897acb095d410dE ud2 .cfi_endproc .globl _bar .p2align 4, 0x90 _bar: .cfi_startproc pushq %rbp .cfi_def_cfa_offset 16 .cfi_offset %rbp, -16 movq %rsp, %rbp .cfi_def_cfa_register %rbp subq $48, %rsp leaq -40(%rbp), %rdx movl $1, %edi movl $1, %esi callq ___rust_alloc testq %rax, %rax je LBB3_1 movb $0, (%rax) movl $1, %edx addq $48, %rsp popq %rbp retq LBB3_1: movq -32(%rbp), %rax movq -24(%rbp), %rcx movq %rcx, -8(%rbp) movq %rax, -16(%rbp) movq -16(%rbp), %rax movq -8(%rbp), %rcx movq %rcx, -24(%rbp) movq %rax, -32(%rbp) leaq -40(%rbp), %rdi callq __ZN5alloc4heap15exchange_malloc28_$u7b$$u7b$closure$u7d$$u7d$17h1d897acb095d410dE ud2 .cfi_endproc .subsections_via_symbols

Was the code change here also still necessary to get the same size reductions?

alexcrichton · 2018-04-02T16:17:21Z

I'm personally ok on adding #[inline] to functions that otherwise wouldn't be inlined across crates (e.g. the str-related shims here) but I'm hesitant to add #[inline] to annotations that are already inlined across crates (aka generics). Optimizations like ThinLTO should already take care of inlining there when necessary, and if something is determined to not be inlined then I'd err on the side of trusting LLVM rather than forcing it to inline which can run the risk of producing suboptimal code in terms of size and such.

nox · 2018-04-08T10:46:13Z

src/liballoc/vec.rs

    /// assert_eq!(slice.into_vec().capacity(), 3);
    /// ```
    #[stable(feature = "rust1", since = "1.0.0")]
+    #[inline(always)]


OTOH, removing this one makes the into_boxed_slice calls not be optimised anymore when the function is duplicated. Even just #[inline] makes rustc not inline the calls anymore.

Hm so in general #[inline(always)] is an antipattern as it can cause compile time to baloon very quickly. Mind if I poke around a bit at the examples you were looking at to see what's going on?

Sure!

#![crate_type = "lib"] #[no_mangle] pub fn foo() -> Box<[u8]> { vec![0].into_boxed_slice() } #[no_mangle] pub fn bar() -> Box<[u8]> { vec![0].into_boxed_slice() }

Hm ok I'm tempted though to leave this as-is and let LLVM decide these sorts of things, we've had bad experiences in the past optimizing too much for microbenchmarks unfortunately

Did you mean this specific #[inline(always)] attribute, or the whole PR? Either is fine with me, to be heard.

Oh just this one particular instance of the attribute. The function is generic so it's already candidate to be inlined everywhere, and it sounds like #[inline] doesn't otherwise work here so #[inline(always)] is going directly against LLVM's heuristics, which has caused a lot of problems for us historically

alexcrichton · 2018-04-16T14:43:30Z

@bors: r+ rollup

Thanks!

bors · 2018-04-16T14:43:31Z

📌 Commit b59fa0d has been approved by alexcrichton

Inline most of the code paths for conversions with boxed slices This helps with the specific problem described in rust-lang#49541, obviously without making any large change to how inlining works in the general case. Everything involved in the conversions is made `#[inline]`, except for the `<Vec<T>>::into_boxed_slice` entry point which is made `#[inline(always)]` after checking that duplicating the function mentioned in the issue prevented its inlining if I only annotate it with `#[inline]`. For the record, that function was: ```rust pub fn foo() -> Box<[u8]> { vec![0].into_boxed_slice() } ``` To help the inliner's job, we also hoist a `self.capacity() != self.len` check in `<Vec<T>>::shrink_to_fit` and mark it as `#[inline]` too.

Rollup of 8 pull requests Successful merges: - #49555 (Inline most of the code paths for conversions with boxed slices) - #49606 (Prevent broken pipes causing ICEs) - #49646 (Use box syntax instead of Box::new in Mutex::remutex on Windows) - #49647 (Remove `underscore_lifetimes` and `match_default_bindings` from active feature list) - #49931 (Fix incorrect span in `&mut` suggestion) - #49959 (rustbuild: allow building tools with debuginfo) - #49965 (Remove warning about f64->f32 cast being potential UB) - #49994 (Remove unnecessary indentation in rustdoc book codeblock.) Failed merges:

rust-highfive assigned KodrAus Apr 1, 2018

rust-highfive assigned alexcrichton and unassigned KodrAus Apr 1, 2018

alexcrichton reviewed Apr 2, 2018

View reviewed changes

emilyalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 2, 2018

shepmaster added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 7, 2018

Remove inline on Vec::shrink_to_fit as asked by Alex

d4dff03

nox commented Apr 8, 2018

View reviewed changes

nox added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 8, 2018

Remove #[inline(always)] on Vec::into_boxed_slice

b59fa0d

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 Apr 16, 2018

kennytm mentioned this pull request Apr 16, 2018

Rollup of 8 pull requests #50003

Merged

bors merged commit b59fa0d into rust-lang:master Apr 16, 2018

Uh oh!

Conversation

nox commented Apr 1, 2018

Uh oh!

eddyb commented Apr 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Apr 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Apr 16, 2018

Uh oh!

bors commented Apr 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants