-
Notifications
You must be signed in to change notification settings - Fork 226
Add CowStr
enum
#615
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.
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
base: main
Are you sure you want to change the base?
Add CowStr
enum
#615
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll want the unit tests in the same PR, to ensure they don't get forgotten.
Can you please motivate a bit more the reasons for this PR (Through a benchmark for example) ? Here it looks like |
Hi @zeenix ,
Thanks for putting time into my PR. |
Hi @sgued, Thanks for putting time to my PR |
Thanks for addressing my concerns. 👍
I think you may have forgotten to push the tests cause I don't see any. 🤔 |
Hey @zeenix I have added the missing tests, sorry for the inconvenience! |
You still haven't justified the benefits for this. What is a use-case where the performance or memory usage here is noticeable? |
Thanks for the review. @sgued, I am now aware that If you are fine with it! then, I can add a Criterion benchmark comparing:
When the ‘no-change’ path dominates, this eliminates most copying. That can reduce cycle count and energy on MCUs. If the caller ultimately always needs an owned value, then I agree CowStr doesn’t help in that situation. If this conditional-clone use-case is not compelling for heapless itself, I’m OK Thanks allot for putting time into my PR :D And sorry for taking your time. |
Please provide such a benchmark. |
Complete agree with @sgued here. This needs a very compelling case. |
Hey @zeenix and @sgued thanks again for putting time to my PR! I have added a cow_str.rs in a new directory named
And looking at the terminal results it seems like in allot of places performance has regressed, many places performance has no change, and in 2 to 3 places performance has improved! I am including a snippet of the terminal output of the TL;DR: No significant change: Many benchmarks show no statistically significant difference. Improvements: A few cases improved (e.g. cowstr_creation/owned_short, cowstr_from/from_string). Terminal output:
Looking at the very few improvements and lots of regressions, I'd appreciate your guidance upon, if I should:
|
benches/cow_str.rs
Outdated
_ => unreachable!(), | ||
}; | ||
|
||
match size { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of matching, why not just split this into another function that takes size as a const generic?
@KushalMeghani1644 Thanks so much for looking into extensive benchmarking. 👍 I have to admit that I'm having a bit of difficulty understanding what exactly is being compared. I think it would be much easier to measure and see the actual benefits (if any) of this PR, if you could add the benchmarks that only use the existing API and then modified to make use of BTW, please do not open a new PR for this. Just force push to this branch. |
This commit adds criterion benchmarks for heapless String operations to establish a performance baseline. These benchmarks measure: - String cloning performance - String creation with various sizes - String access operations These will be used to compare against the CowStr implementation.
5c0029b
to
ea3bd5b
Compare
Hey @zeenix I have changed everything as you requested: Changes Made:
Commit 1: "Add baseline benchmarks for String operations" - Contains benchmarks using only the existing String API Commit 2: "Add CowStr clone-on-write string type" - Adds the complete CowStr implementation with tests Commit 3: "Add CowStr benchmarks comparing against String baseline" - Adds comprehensive CowStr benchmarks that can be compared against the baseline This structure makes it easy to run benchmarks before and after the CowStr
Both benchmark files compile successfully and all tests pass. The benchmarks can now be run to compare String cloning performance against CowStr's clone-on-write behavior. Thanks a lot for putting time to my PR :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for updating the PR. However, you're adding new benchmarks for the CowStr in the last commit. I asked for modifying the benchmarks to make use of the CowStr, so it's easier to see/observe the difference.
Moreover, we have a suspicion that you're using AI tools for not only the coding part but also writing your comments. While I have no objections on making use of AI tools for the code (as long as it's good quality code), I think using AI to reply to us, is unacceptable. Please ensure that your following comments on this PR are written exclusively by you.
Overview
This PR adds a
Cow<'a, N, LenT>
type to the heapless crate, enabling efficient clone-on-write strings compatible with heapless data structures.Features
Cow::Borrowed(&StringView<LenT>)
for zero-copy references.Cow::Owned(String<N, LenT>)
for owned heapless strings.to_owned()
,as_str()
,is_borrowed()
,is_owned()
.From<&StringView<LenT>>
andFrom<String<N, LenT>>
for easy construction.Borrow<str>
for compatibility with APIs expecting&str
.Motivation
Currently, heapless strings require manual cloning or reference handling.
Cow
simplifies this by providing a single type that can efficiently represent either a borrowed or owned string.Testing
Verified compilation and basic API usage. Unit tests for
Cow
will follow in a separate PR.Cow
type #582