-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
refactor: Switch from termcolor to anstream #12751
Conversation
r? @ehuss (rustbot has picked a reviewer for you, use r? to override) |
We are already getting `anstream` through `clap`, so this is no extra cost and let's us drop some dependencies. The `anstream` implementation is also orders of magnitude faster (last I benchmarked)
`cargo report` calls `Shell::print_ansi_stdout`. Previously, it didn't always strip the colors when needed (e.g. `Write` is used). Now it does, so don't need to special case this when generating the report.
These messages will eventually be forwarded to `Shell` which will strip as needed, making it so we don't need to strip here anymore.
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.
Thanks! I like the cleaner code fewer dependencies.
let mut buffer = Vec::new(); | ||
if justified { | ||
write!(&mut buffer, "{style}{status:>12}{reset}")?; | ||
} else { | ||
write!(&mut buffer, "{style}{status}{reset}{bold}:{reset}")?; | ||
} | ||
match message { | ||
Some(message) => writeln!(buffer, " {message}")?, | ||
None => write!(buffer, " ")?, | ||
} | ||
self.stderr().write_all(&buffer)?; |
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.
obviously I'm just some outsider, but I'm surprised by how this PR adds many small allocations. The previous implementation seems to carefully avoid making those allocations.
It seems to me that some "code pain" in cargo is worth it given the huge number of daily cargo invocations.
In this particular case using a Cursor around an array seems like a simple alternative.
let mut buffer = Vec::new(); | |
if justified { | |
write!(&mut buffer, "{style}{status:>12}{reset}")?; | |
} else { | |
write!(&mut buffer, "{style}{status}{reset}{bold}:{reset}")?; | |
} | |
match message { | |
Some(message) => writeln!(buffer, " {message}")?, | |
None => write!(buffer, " ")?, | |
} | |
self.stderr().write_all(&buffer)?; | |
let mut buffer = std::io::Cursor::new([0u8; 1024]); | |
if justified { | |
write!(&mut buffer, "{style}{status:>12}{reset}")?; | |
} else { | |
write!(&mut buffer, "{style}{status}{reset}{bold}:{reset}")?; | |
} | |
match message { | |
Some(message) => writeln!(buffer, " {message}")?, | |
None => write!(buffer, " ")?, | |
} | |
self.stderr() | |
.write_all(&buffer.get_ref()[..buffer.position() as usize])?; |
I'm not entirely sure why writing into a separate buffer before actually writing to stderr is required here. If it turns out that separate buffer is not needed, then this could work?
let mut buffer = Vec::new(); | |
if justified { | |
write!(&mut buffer, "{style}{status:>12}{reset}")?; | |
} else { | |
write!(&mut buffer, "{style}{status}{reset}{bold}:{reset}")?; | |
} | |
match message { | |
Some(message) => writeln!(buffer, " {message}")?, | |
None => write!(buffer, " ")?, | |
} | |
self.stderr().write_all(&buffer)?; | |
let stderr = self.stderr(); | |
if justified { | |
write!(stderr, "{style}{status:>12}{reset} ")?; | |
} else { | |
write!(stderr, "{style}{status}{reset}{bold}:{reset} ")?; | |
} | |
if let Some(message) = message { | |
write!(stderr, "{message}")?; | |
} |
obviously this is not the main point of this PR, it just surprised me. This is just one example, the same pattern seems to occur in a bunch of places and the allocations can be removed in similar ways.
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.
The original buffering was added in #12727 to workaround the fact that we aren't locking the stream (unsure why lock wasn't used instead).
The difference between #12727 and this is that we aren't reusing the buffer.
I expect allocations aren't our biggest concern here
- In most cases, we are writing to the screen which has its own performance concerns
- A good number of calls to these functions will already be having to do a
format!
. I've found the biggest difference is between 'no allocation" and "allocation" code paths. The difference between "3 allocations" and "4 allocations" is small.
If allocations were to be an issue, our options are
- Cache the
Vec
inside ofShellOutput
- Extend
anstream
to allow locking without transferring ownershipAutoStream::lock
transfers ownership, which doesn't work here, because (1)AutoStream
is stateful and it adds a lot of complexity in both the implementation and the caller (the locked stream needs a&mut
back to the original stream) (2) most of the time none of this will matter but when it doesn, I found enough cases want access to an owned stream
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.
Since the number of logged status messages is relatively small, I don't think there's a going to be any measurable performance impact from this allocation. Using a fixed-length stack buffer would avoid the allocation, but also uses an unnecessary amount of stack space in most cases, and limits message length.
bd4654f
to
a770d36
Compare
Thanks! @bors r+ |
☀️ Test successful - checks-actions |
Update cargo 4 commits in e6aabe8b3fcf639be3a5bf68e77853bd7b3fa27d..59596f0f31a94fde48b5aa7e945cd0b7ceca9620 2023-09-26 16:31:53 +0000 to 2023-09-29 19:29:17 +0000 - refactor: Switch from termcolor to anstream (rust-lang/cargo#12751) - Add missing `strip` entries in `dev` and `release` profiles. (rust-lang/cargo#12748) - Add better suggestion for the unsupported silent flag (rust-lang/cargo#12723) - docs(ref): Establish publish best practices (rust-lang/cargo#12745) r? ghost
Update cargo 4 commits in e6aabe8b3fcf639be3a5bf68e77853bd7b3fa27d..59596f0f31a94fde48b5aa7e945cd0b7ceca9620 2023-09-26 16:31:53 +0000 to 2023-09-29 19:29:17 +0000 - refactor: Switch from termcolor to anstream (rust-lang/cargo#12751) - Add missing `strip` entries in `dev` and `release` profiles. (rust-lang/cargo#12748) - Add better suggestion for the unsupported silent flag (rust-lang/cargo#12723) - docs(ref): Establish publish best practices (rust-lang/cargo#12745) r? ghost
refactor(shell): Write at once rather than in fragments ### What does this PR try to resolve? For `cargo add` and `cargo search`, rather than expose `termcolor`s API, we wrote a styled fragment at a time. This is no longer needed as of #12751 because we switched from termcolor to anstream which decorates `stdout` and `stderr` with any of the logic we need to adapt to the configured terminal. . ### How should we test and review this PR? Ran the commands locally to verify styled output. Tests verify unstyled output. ### Additional information
feat: Make browser links out of HTML file paths This provides an alternative to `--open`, where supported. Note: because we are relying on `supports-hyperlinks`, we are getting `FORCE_HYPERLINK` for "free". Unsure whether it and the current policy (it gets overridden by `term.hyperlinks`) is something we want. `FORCE_HYPERLINK` mirrors the npm package's behavior of the same name (see zkat/supports-hyperlinks#2) though though I also found reading of it in ohmyzsh, a go terminal library and many more places. Similarly, #12751 added indirect, undocumented support for community environment variables. Fixes #12888
What does this PR try to resolve?
anstream
asks the question "what if you tookfwdansi
and removedtermcolor
underneath it. It wraps output streams, adapting ANSI escape codes to what is neededBenefits
write!
with styled text rather than the back-and-forth between colors and writing that termcolor needsShell
can accept styled messages and adapt them as neededSide effects
Fixes #12627
How should we test and review this PR?
This is broken up by commits for easier browsing.
However, as there aren't really tests for colored output, this needs hand inspection to verify
Additional information
This allowed us to remove the need for stripping ansi escape codes completely. Even if it didn't, it exposes its internal stripping API for reuse, saving on a dependency and being significantly faster.