Allocate a lot less #202
Allocate a lot less #202
Conversation
|
Reducing unnecessary allocations sounds good. I'll review this later. You may want to check out the |
I would wait until my next commit, I think I have an idea how to do the padding without allocating, but I have to rework my solution for this quite a bit |
|
Added now padding without alloc Pro:
Con:
|
|
Notice, this will break Allow colorizing any type implementing a formatter trait The reason is, that at least If the input would be generic: |
|
Also, since your goal is not to be alloc free, you could just call T::to_string() and then format it |
| /// Formatting *must* respect padding | ||
| /// | ||
| /// The added normal str in the input for comparison to rust-std | ||
| mod formmating { |
There was a problem hiding this comment.
| mod formmating { | |
| #[cfg(test)] | |
| mod formatting { |
There was a problem hiding this comment.
Reason?
mod formatting is child of mod tests which is #[cfg(tests)]
src/color.rs
Outdated
| const fn to_bg_static_str(self) -> Result<&'static str, (u8, u8, u8)> { | ||
| match self { | ||
| Self::Black => Ok("40"), | ||
| Self::Red => Ok("41"), | ||
| Self::Green => Ok("42"), | ||
| Self::Yellow => Ok("43"), | ||
| Self::Blue => Ok("44"), | ||
| Self::Magenta => Ok("45"), | ||
| Self::Cyan => Ok("46"), | ||
| Self::White => Ok("47"), | ||
| Self::BrightBlack => Ok("100"), | ||
| Self::BrightRed => Ok("101"), | ||
| Self::BrightGreen => Ok("102"), | ||
| Self::BrightYellow => Ok("103"), | ||
| Self::BrightBlue => Ok("104"), | ||
| Self::BrightMagenta => Ok("105"), | ||
| Self::BrightCyan => Ok("106"), | ||
| Self::BrightWhite => Ok("107"), | ||
| Self::TrueColor { r, g, b } => Err((r, g, b)), | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn to_bg_fmt(self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> { | ||
| match self.to_bg_static_str() { | ||
| Ok(s) => f.write_str(s), | ||
| Err((r, g, b)) if !truecolor_support() => Self::TrueColor { r, g, b } | ||
| .closest_color_euclidean() | ||
| .to_fg_fmt(f), | ||
| Err((r, g, b)) => write!(f, "48;2;{r};{g};{b}"), |
There was a problem hiding this comment.
It looks like you used Result as a clever way to "unwrap" the truecolor if it is one. Here's how I'm thinking it should be implemented instead, for clarity.
// Note that we're now borrowing, since we don't have a reason to take ownership and force a copy
const fn to_bg_static_str(&self) -> &str {
Self::Black => "40",
// Implement the other simple colors...
// Panicking is OK because this is internal
Self::TrueColor { r, g, b } => unreachable!("Shouldn't be called on True Color"),
}
pub(crate) fn to_bg_fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> {
if let Self::TrueColor { r, g, b } = self {
// Check for support, write value
// Can possibly be split into a "write_true_color()"
return Ok(());
}
// If we're here, we know the color can be converted to a static string
f.write_str(self.to_bg_static_str())
}There was a problem hiding this comment.
My intent was inspired by something like tokio's send method.
I try to convert the color in a static str. If it is possible, I return it, if not, I return rgb (since the color is not statically known).
The reason, I do not need a second match or any unreachable. I personally dislike the use of unreachable if it can be avoided easily.
Also, for borrowing the color, this is an internal api (so no stability needed) and Color is smaller than a pointer. So it is cheaper to hand over the color instead of an pointer. See this clippy::trivially_copy_pass_by_ref
No problem, it is quite a big change with a lot of logic (and possible logical errors). I recommend to review the changes when the ci run through, because the changed code was also tested with the old tests. There is still a allocation in Style. I have a fix for it, but and also added more tests. But for testing, I would like to add itertools as a dev dependency. Am I allowed to do that? If not I can just revert the commit |
|
Hello @spenserblack, Sorry for the ping 😅 . I tested this PR which seems to work well, would it be possible for you to look into potentially merge it and make a new release? |
|
I've run fuzzing with two test setups: the first with 30,283,522,069 iterations, and the second — a more sophisticated version — with 8,031,822,837 iterations. In both cases, I tested the colored_original (current commit, 68761c1) and colored_new (this PR, 2497bd0). The only changes made were the removal of #[non_exhaustive] on ColoredString and making Style public to allow external construction — everything else remained unchanged. No issues were found during fuzzing. The only open question was my 'clever' way to unwrap truecolor. But I also added an own Error type (NotStaticColor) for this case, since AnsiColor was also added. So I think this should be fine, and in my opinion better than using unreachable!, which could theoretically panic. |
This change reduced around ~40-50% of allocations for intentrace.
Changed the implementation to write into the given buffer instead of allocating an own buffer.
If you do not use padding, Display should not allocate anymore.
I think the test should pass, but I'm not familiar with the test setup, so check again please
Pro:
Con:
Allocations bench-marked with'
valgrind --tool=dhat intentrace -q lsAllocations went from 296,761 bytes to 164,552 bytesNotice that the benchmark is just here to give an idea, not to be reproducible or credible
TODOS: