Add && Impl new trait ToCompactStr #16

NobodyXu · 2021-09-28T12:41:03Z

Similar to std::string::ToString, ToCompactStr convert any value implements fmt::Display into a CompactStr efficiently.

However, due to the fact that Arc::<[MaybeUninit<T>]> is still unstable, there is no way to implement HeapString efficiently without first creating a String, then copy it onto the buffer and deallocate the original String.

It seems that the compiler may not be able to optimize these calls, especially with std::format! calling format.

Signed-off-by: Jiahao XU Jiahao_XU@outlook.com

ParkMyCar · 2021-09-30T14:57:38Z

Thanks for the PR!

I really like the idea of a ToCompactStr trait. But you're right, without the nightly compiler we can't do this very efficiently. The HeapString representation will be changing soon, see #12, I plan to spend some time on it this weekend. Let's hold off on landing this until we make the changes to HeapString

NobodyXu · 2021-10-01T03:00:52Z

Sure, I will wait for #12

ParkMyCar · 2022-03-31T15:18:39Z

Hey @NobodyXu, now that CompactStr is more stable, would you want to take another shot at this?

I think the idea of a ToCompactStr trait is great, I'm a bit apprehensive of manually implementing it for any type that implements Display though because of performance. Maybe we could define the trait, and then manually implement for a few common types like all the number types (e.g. u8, isize, f32, etc), and then maybe bool and char? IMO that would go a fairly long way for common use cases, what do you think?

For efficiently converting number types to strings take a look at itoa and ryu

NobodyXu · 2022-04-01T12:07:21Z

Hey @NobodyXu, now that CompactStr is more stable, would you want to take another shot at this?

Yes, sure.

I think the idea of a ToCompactStr trait is great, I'm a bit apprehensive of manually implementing it for any type that implements Display though because of performance. Maybe we could define the trait, and then manually implement for a few common types like all the number types (e.g. u8, isize, f32, etc), and then maybe bool and char? IMO that would go a fairly long way for common use cases, what do you think?

Hmmm, I don't like the idea of implementing only for common types, as it would make ToCompactStr less useful and
provide different semantics compared to ToString.

For common number types, maybe we can provide a optional specialization for them using
castaway and gate it using a feature flag?

ParkMyCar · 2022-04-01T21:43:12Z

Hmmm, I don't like the idea of implementing only for common types, as it would make ToCompactStr less useful and
provide different semantics compared to ToString.

Fair enough, that makes sense to me!

Ugh I wish specialization was stable. Regardless, castaway looks pretty cool, let's give it a shot!

NobodyXu · 2022-04-03T03:11:09Z

Hmmm, I don't like the idea of implementing only for common types, as it would make ToCompactStr less useful and
provide different semantics compared to ToString.

Fair enough, that makes sense to me!

Ugh I wish specialization was stable. Regardless, castaway looks pretty cool, let's give it a shot!

I am quite busy recently, so it might take me some time to implement it.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu · 2022-04-03T14:15:42Z

Sorry I accidentally closed this PR, now it is reopened.

NobodyXu · 2022-04-03T14:19:23Z

@ParkMyCar I have implemented trait ToCompactStr without specialisation using castaway.

I will implement the specialisation later, but now you can probably start looking at my code and providing me with feedback so that we can ensure the implementation is correct before doing any optimization.

ParkMyCar

Thanks for doing this work!

compact_str/src/lib.rs

compact_str/src/utility.rs

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

compact_str/src/repr/mod.rs

Impl new function `InlineString::from_fmt` to avoid copying the buffer for `InlineString`. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

into `InlineString::from_fmt` Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Instead of using `std::io::Cursor`. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

`[u8; _]::as_mut_slice` is only present in rust >= 1.57.0 Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu · 2022-04-07T05:27:59Z

For efficiently converting number types to strings take a look at itoa and ryu

I am doubtful on using ryu as it might give different output compared to the builtin format!, I think this might break some code.

NobodyXu · 2022-04-07T06:47:02Z

Hmmm, I don't like the idea of implementing only for common types, as it would make ToCompactStr less useful and
provide different semantics compared to ToString.

Fair enough, that makes sense to me!

Ugh I wish specialization was stable. Regardless, castaway looks pretty cool, let's give it a shot!

I just realized that castaway requires the type to be of 'static lifetime due to std::any::TypeId::of requires static lifetime, which is impossible to use within ToCompactStr.

I am now thinking of adding another trait FromBuiltinTypes.

It is just unfortunate that Rust doesn't have specialisation stabilised.

NobodyXu · 2022-04-08T04:23:48Z

I also asked the question sagebind/castaway#5 on castaway to see if there is any alternative solution.

Edit:

Turns out it is actually feasible sagebind/castaway#6

NobodyXu · 2022-04-21T00:51:02Z

Hmmm, I don't like the idea of implementing only for common types, as it would make ToCompactStr less useful and
provide different semantics compared to ToString.

Fair enough, that makes sense to me!
Ugh I wish specialization was stable. Regardless, castaway looks pretty cool, let's give it a shot!

I just realized that castaway requires the type to be of 'static lifetime due to std::any::TypeId::of requires static lifetime, which is impossible to use within ToCompactStr.

I am now thinking of adding another trait FromBuiltinTypes.

It is just unfortunate that Rust doesn't have specialisation stabilised.

With sagebind/castaway#6 merged in and released as https://github.com/sagebind/castaway/releases/tag/0.2.0, this is no longer an issue.

I will continue my work on specialisation for ToCompactStr.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

`castaway::cast!` cannot cast reference of `T: ?Sized` to primitive types Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Use `Sink(0)` directly to make optimization easier. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

to avoid using macro `count!` in `Repr::from_fmt`, which uses `core::format_args!`, making it harder to optimize. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

For `LifetimeFree` impl for `NonZeroUsize`. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Test different float values in the same `assert_eq` so that if one `assert_eq`, we can obtain value of the formatting of all float values we provided. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu · 2022-04-23T15:15:33Z

~~I just tried to reproduce this on my own x86_64 linux box by running:~~

~~cross +nightly test --release --all-features --manifest-path=compact_str/Cargo.toml --target powerpc-unknown-linux-gnu~~

~~however, all tests passed without problem...~~

Sorry I used the wrong command.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

ParkMyCar

Thanks for all the work here @NobodyXu! I've been pretty busy lately so haven't had a chance to look super in-depth, but I think we're getting close!

.github/workflows/msrv.yml

compact_str/Cargo.toml

compact_str/src/repr/inline.rs

compact_str/src/lib.rs

compact_str/src/tests.rs

compact_str/src/to_compact_str_specialisation.rs

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Instead of calling `Repr::from_fmt`, then forward it to `Inline::from_fmt` or formatting into preallocated `String`, this commit uses `CompactStr::with_capacity` to preallocate required space ahead of time to avoid repeated heap allocation and copying, then use `fmt::Write` implementation for `CompactStr` to format value into the `CompactStr` with preallocated space. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

- Module `io` only present in `std`, while `fmt` present in `core`. - `io::Write` requires additional `flush` method to be implemented. - `io::Write::write` requires the len of bytes written to be returned. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

by implementing `fmt::Write::write_char` Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

To make this PR easier to review. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

ParkMyCar

Thanks for all the hard work here @NobodyXu and making all the changes I asked for!

Looks like there's just one failing test on PowerPC Little Endian with float formatting? If we can fix the test that would be great, otherwise I'm fine with ignoring it. Once you make that change we can merge!

Thanks again for all the work! 🙂

NobodyXu · 2022-04-25T23:56:38Z

Looks like there's just one failing test on PowerPC Little Endian with float formatting? If we can fix the test that would be great, otherwise I'm fine with ignoring it. Once you make that change we can merge!

Unfortunately that is a compiler bug rust-lang/rust#96306 which I cannot fix.

@ParkMyCar I think this PR is ready for merging as the failure on powepc64le has nothing to do with this PR.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

ParkMyCar · 2022-04-26T14:03:56Z

Merging, thanks @NobodyXu!

NobodyXu · 2022-05-02T02:20:15Z

@ParkMyCar Now that we have more unit tests and fuzzing added for ToCompactStr, maybe it is a good time to release a new minor version?

Mayhem fork

NobodyXu closed this Apr 3, 2022

Impl trait ToCompactStr

f00cf33

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu reopened this Apr 3, 2022

ParkMyCar requested changes Apr 3, 2022

View reviewed changes

compact_str/src/lib.rs Outdated Show resolved Hide resolved

compact_str/src/lib.rs Outdated Show resolved Hide resolved

compact_str/src/lib.rs Outdated Show resolved Hide resolved

compact_str/src/utility.rs Outdated Show resolved Hide resolved

NobodyXu added 2 commits April 4, 2022 12:34

Optimize Repr::from_fmt: Optimize formatting to String

a5af400

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Fix doc of ToCompactStr::to_compact_str

348fbcd

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu mentioned this pull request Apr 4, 2022

feature: Immutable shared version of CompactStr? #80

Open

ParkMyCar reviewed Apr 6, 2022

View reviewed changes

compact_str/src/repr/mod.rs Outdated Show resolved Hide resolved

NobodyXu added 5 commits April 7, 2022 13:09

Optimize Repr::from_fmt for InlineString.

6252852

Impl new function `InlineString::from_fmt` to avoid copying the buffer for `InlineString`. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Merge branch 'main' into main

53b35d4

Refactor: Inline utility::format_into_buffer

339def9

into `InlineString::from_fmt` Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Simplify InlineString::from_fmt impl: write! to buffer directly

67b76f2

Instead of using `std::io::Cursor`. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Fix MSRV issue caused by InlineString::from_fmt

b8bf0f0

`[u8; _]::as_mut_slice` is only present in rust >= 1.57.0 Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu requested a review from ParkMyCar April 7, 2022 03:42

NobodyXu mentioned this pull request Apr 15, 2022

feature: easy integer to CompactStr #82

Closed

NobodyXu added 3 commits April 21, 2022 12:14

Add optional dep castaway v0.2

8dd55f5

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Add feature to-compact-str-specialisation

48540eb

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Rm ?Sized relaxtion for ToCompactStr

056ef48

`castaway::cast!` cannot cast reference of `T: ?Sized` to primitive types Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

NobodyXu added 9 commits April 22, 2022 12:38

Optimize Sink::count: Avoid using Sink::default

59884e9

Use `Sink(0)` directly to make optimization easier. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Accept fmt::Display param in Sink::count

8f4fd0e

to avoid using macro `count!` in `Repr::from_fmt`, which uses `core::format_args!`, making it harder to optimize. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Rm unused derive from utility::Sink

4d53402

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Make Sink::count a free function

1fa20a9

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Set vis of utility::Sink to private

a048c7d

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Fix code formatting of utility::tests::count!

3ec342b

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Bump dep castaway to v0.2.1

c42d514

For `LifetimeFree` impl for `NonZeroUsize`. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Fix int spec for NonZeroUsize in ToCompactStr

d310ef0

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Improve test for float spec of ToCompactStr

9f6d7c9

Test different float values in the same `assert_eq` so that if one `assert_eq`, we can obtain value of the formatting of all float values we provided. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

This was referenced Apr 22, 2022

Ryu format 3.2 as 0.0 on powerpc64le dtolnay/ryu#47

Closed

3.2_f32 formatted as 0 on powerpc64le rust-lang/rust#96306

Closed

Rm workaround for ryu on powerpc64le

8ed9b51

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Rm job linux_powerpc_be_64bit from CI cross_platform

ab26a93

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

ParkMyCar requested changes Apr 24, 2022

View reviewed changes

NobodyXu added 8 commits April 25, 2022 13:22

Rm features for specialisation

1b14f26

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Rename to_compact_str! to format_compact! in src/tests.rs

9f258bf

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Optimize fmt::Write impl for Sink

4dd68b0

by implementing `fmt::Write::write_char` Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Revert modification to workflow msrv.yml

e2e06f8

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Reduce the diff by using cargo +nightly fmt --all

c0e5d00

To make this PR easier to review. Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

Optimize to_compact_str_specialised: Use chained if

305e11c

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

ParkMyCar approved these changes Apr 25, 2022

View reviewed changes

Rm buggy impl of utility::Sink::Write::write_char

5c74de3

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>

ParkMyCar merged commit ad93a70 into ParkMyCar:main Apr 26, 2022

nottirb pushed a commit to nottirb/compact_str that referenced this pull request May 15, 2022

Merge pull request ParkMyCar#16 from samsaq/mayhem-fork

07eeae4

Mayhem fork

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add && Impl new trait ToCompactStr #16

Add && Impl new trait ToCompactStr #16

NobodyXu commented Sep 28, 2021

ParkMyCar commented Sep 30, 2021

NobodyXu commented Oct 1, 2021

ParkMyCar commented Mar 31, 2022

NobodyXu commented Apr 1, 2022

ParkMyCar commented Apr 1, 2022 •

edited

NobodyXu commented Apr 3, 2022

NobodyXu commented Apr 3, 2022

NobodyXu commented Apr 3, 2022

ParkMyCar left a comment

NobodyXu commented Apr 7, 2022

NobodyXu commented Apr 7, 2022 •

edited

NobodyXu commented Apr 8, 2022 •

edited

NobodyXu commented Apr 21, 2022 •

edited

NobodyXu commented Apr 23, 2022 •

edited

ParkMyCar left a comment

ParkMyCar left a comment

NobodyXu commented Apr 25, 2022 •

edited

ParkMyCar commented Apr 26, 2022

NobodyXu commented May 2, 2022

Add && Impl new trait ToCompactStr #16

Add && Impl new trait ToCompactStr #16

Conversation

NobodyXu commented Sep 28, 2021

ParkMyCar commented Sep 30, 2021

NobodyXu commented Oct 1, 2021

ParkMyCar commented Mar 31, 2022

NobodyXu commented Apr 1, 2022

ParkMyCar commented Apr 1, 2022 • edited

NobodyXu commented Apr 3, 2022

NobodyXu commented Apr 3, 2022

NobodyXu commented Apr 3, 2022

ParkMyCar left a comment

Choose a reason for hiding this comment

NobodyXu commented Apr 7, 2022

NobodyXu commented Apr 7, 2022 • edited

NobodyXu commented Apr 8, 2022 • edited

NobodyXu commented Apr 21, 2022 • edited

NobodyXu commented Apr 23, 2022 • edited

ParkMyCar left a comment

Choose a reason for hiding this comment

ParkMyCar left a comment

Choose a reason for hiding this comment

NobodyXu commented Apr 25, 2022 • edited

ParkMyCar commented Apr 26, 2022

NobodyXu commented May 2, 2022

ParkMyCar commented Apr 1, 2022 •

edited

NobodyXu commented Apr 7, 2022 •

edited

NobodyXu commented Apr 8, 2022 •

edited

NobodyXu commented Apr 21, 2022 •

edited

NobodyXu commented Apr 23, 2022 •

edited

NobodyXu commented Apr 25, 2022 •

edited