Skip to content

Make BytesMut::new a const fn #472

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 2 commits into from

Conversation

coolreader18
Copy link
Contributor

This might depend on rust-lang/rust#65801 for the guarantee that Vec::new() is a dangling pointer, but in practice this works and std probably wouldn't change.

@coolreader18
Copy link
Contributor Author

I also inlined functions as I was originally going to fix BytesMut::new() being compiled as:

foo::foo:
 push    r14
 push    rbx
 sub     rsp, 24
 mov     rbx, rdi
 mov     qword, ptr, [rsp], 1
 xorps   xmm0, xmm0
 movups  xmmword, ptr, [rsp, +, 8], xmm0
 mov     edi, 1
 call    qword, ptr, [rip, +, _ZN5bytes9bytes_mut4vptr17ha5d83dd4c2961c0aE@GOTPCREL]
 mov     r14, rax
 xor     edi, edi
 call    qword, ptr, [rip, +, _ZN5bytes9bytes_mut25original_capacity_to_repr17hedb301e0cc79d76aE@GOTPCREL]
 lea     rax, [4*rax, +, 1]
 mov     qword, ptr, [rbx], r14
 xorps   xmm0, xmm0
 movups  xmmword, ptr, [rbx, +, 8], xmm0
 mov     qword, ptr, [rbx, +, 24], rax
 mov     rax, rbx
 add     rsp, 24
 pop     rbx
 pop     r14
 ret
.LBB1_2:
 mov     rbx, rax
 mov     rdi, rsp
 call    core::ptr::drop_in_place
 mov     rdi, rbx
 call    _Unwind_Resume
 ud2

I'm not sure if making it const fixes that as well, but I'll leave the inlines since I think they make sense anyway.

@coolreader18
Copy link
Contributor Author

Actually, the inlines do matter lol:

diff --git a/src/bytes_mut.rs b/src/bytes_mut.rs
index 4e638c5..591b092 100644
--- a/src/bytes_mut.rs
+++ b/src/bytes_mut.rs
@@ -1260,7 +1260,7 @@ impl Shared {
     }
 }
 
-#[inline]
+// #[inline]
 const fn original_capacity_to_repr(cap: usize) -> usize {
     let width = PTR_WIDTH - ((cap >> MIN_ORIGINAL_CAPACITY_WIDTH).leading_zeros() as usize);
     let max_capacity = MAX_ORIGINAL_CAPACITY_WIDTH - MIN_ORIGINAL_CAPACITY_WIDTH;
foo::foo:
 push    rbx
 mov     rbx, rdi
 xor     edi, edi
 call    qword, ptr, [rip, +, _ZN5bytes9bytes_mut25original_capacity_to_repr17hedb301e0cc79d76aE@GOTPCREL]
 lea     rax, [4*rax, +, 1]
 mov     qword, ptr, [rbx], 1
 xorps   xmm0, xmm0
 movups  xmmword, ptr, [rbx, +, 8], xmm0
 mov     qword, ptr, [rbx, +, 24], rax
 mov     rax, rbx
 pop     rbx
 ret

@coolreader18
Copy link
Contributor Author

Oh, maybe LTO would fix the inlining issue, since it was working when I was testing it as foo as a function in bytes, but not as a separate crate. In any case, this works too.

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! The internal representation of BytesMut is not stable and after #435 it may be difficult to keep const so I would prefer to postpone this for now.

@taiki-e taiki-e closed this Apr 10, 2021
@coolreader18
Copy link
Contributor Author

That makes sense. Would you accept a PR to add some #[inline]s so that the cost of BytesMut::new is as if it was const?

@taiki-e
Copy link
Member

taiki-e commented Apr 10, 2021

Would you accept a PR to add some #[inline]s so that the cost of BytesMut::new is as if it was const?

I would accept a PR to do this.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants