Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 16, 2018
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
360f2f0
Inline most of the code paths for conversions with boxed slices
nox Mar 31, 2018
d4dff03
Remove inline on Vec::shrink_to_fit as asked by Alex
nox Apr 8, 2018
b59fa0d
Remove #[inline(always)] on Vec::into_boxed_slice
nox Apr 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/liballoc/boxed.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,13 +577,15 @@ impl<'a, T: Copy> From<&'a [T]> for Box<[T]> {

#[stable(feature = "box_from_slice", since = "1.17.0")]
impl<'a> From<&'a str> for Box<str> {
#[inline]
fn from(s: &'a str) -> Box<str> {
unsafe { from_boxed_utf8_unchecked(Box::from(s.as_bytes())) }
}
}

#[stable(feature = "boxed_str_conv", since = "1.19.0")]
impl From<Box<str>> for Box<[u8]> {
#[inline]
fn from(s: Box<str>) -> Self {
unsafe { Box::from_raw(Box::into_raw(s) as *mut [u8]) }
}
Expand Down
3 changes: 3 additions & 0 deletions src/liballoc/str.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1811,6 +1811,7 @@ impl str {
/// assert_eq!(*boxed_bytes, *s.as_bytes());
/// ```
#[stable(feature = "str_box_extras", since = "1.20.0")]
#[inline]
pub fn into_boxed_bytes(self: Box<str>) -> Box<[u8]> {
self.into()
}
Expand Down Expand Up @@ -2049,6 +2050,7 @@ impl str {
/// assert_eq!(boxed_str.into_string(), string);
/// ```
#[stable(feature = "box_str", since = "1.4.0")]
#[inline]
pub fn into_string(self: Box<str>) -> String {
let slice = Box::<[u8]>::from(self);
unsafe { String::from_utf8_unchecked(slice.into_vec()) }
Expand Down Expand Up @@ -2307,6 +2309,7 @@ impl str {
/// assert_eq!("☺", &*smile);
/// ```
#[stable(feature = "str_box_extras", since = "1.20.0")]
#[inline]
pub unsafe fn from_boxed_utf8_unchecked(v: Box<[u8]>) -> Box<str> {
Box::from_raw(Box::into_raw(v) as *mut str)
}
1 change: 1 addition & 0 deletions src/liballoc/string.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1587,6 +1587,7 @@ impl String {
/// let b = s.into_boxed_str();
/// ```
#[stable(feature = "box_str", since = "1.4.0")]
#[inline]
pub fn into_boxed_str(self) -> Box<str> {
let slice = self.vec.into_boxed_slice();
unsafe { from_boxed_utf8_unchecked(slice) }
Expand Down
5 changes: 4 additions & 1 deletion src/liballoc/vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,9 @@ impl<T> Vec<T> {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn shrink_to_fit(&mut self) {
self.buf.shrink_to_fit(self.len);
if self.capacity() != self.len {
self.buf.shrink_to_fit(self.len);
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check and report back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

}

/// Shrinks the capacity of the vector with a lower bound.
Expand Down Expand Up @@ -636,6 +638,7 @@ impl<T> Vec<T> {
/// assert_eq!(slice.into_vec().capacity(), 3);
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[inline(always)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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()
}

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

pub fn into_boxed_slice(mut self) -> Box<[T]> {
unsafe {
self.shrink_to_fit();
Expand Down