-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
rework std.Progress #20059
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
rework std.Progress #20059
Conversation
Very nice! This would probably close this issue |
@squeek502 do you have any interest in helping out on the Windows side of things here? There are two challenges:
I think the second one is the more difficult one, but even with only (1) solved, it would still be useful. There would not be visibility into child processes but it would be a good start. |
Sure, but it might be a few days before I can give it proper attention. |
I lied; I worked on the Windows console API stuff and got a proof-of-concept working but it's a bit weird. The implementation is here: (all the IPC stuff is commented out as a quick fix to get it to compile on Windows) When the console supports ANSI escape codes, then ANSI escape codes are used and it works as expected: progress_ansi.mp4And when the console does not support ANSI escape codes, it falls back to the Windows console API: (the flickering only happened while I was recording with OBS; there's very minimal flickering when not recording) progress_consoleapi.mp4Note that there is a ► character at the start of the progress when using the Windows API. This is because AFAICT there's no way to go to/find the start of a line if the line is being wrapped. That is, carriage returns only bring you to the start of the row, not the start of the line. So, the workaround is to write out a special character at the start of the progress and then take advantage of the The important part of the implementation is here: with some commented out alternate implementations below it that worked somewhat but had issues. Also, when falling back to the Windows console API it currently assumes code page 437 (the default code page for consoles) is the active code page and uses that for the box drawing characters. Some TODOs about that are here: |
Looks great! |
753ef4d
to
de82e8d
Compare
New design ideas: * One global instance, don't try to play nicely with other instances except via IPC. * One process owns the terminal and the other processes communicate via IPC. * Clear the whole terminal and use multiple lines. What's implemented so far: * Query the terminal for size. * Register a SIGWINCH handler. * Use a thread for redraws. To be done: * IPC * Handling single threaded targets * Porting to Windows * More intelligent display of the progress tree rather than only using one line.
Move the mutex into the nodes Track the whole tree instead of only recently activated node
This time, we preallocate a fixed set of nodes and have the user-visible Node only be an index into them. This allows for lock-free management of the node storage. Only the parent indexes are stored, and the update thread makes a serialized copy of the state before trying to compute children lists. The update thread then walks the tree and outputs an entire tree of progress rather than only one line. There is a problem with clearing from the cursor to the end of the screen when the cursor is at the bottom of the terminal.
the clear, save, restore thing doesn't work when the terminal is at the bottom
so that the previous message can be used when the pipe is empty. prevents flickering
and properly dup2 the file descriptor to make it handle the case when other files are already open
also fix handling of BrokenPipe also fix continuing wrong loop in error conditions
The update thread was sometimes reading the special state and then incorrectly getting 0 for the file descriptor, making it hang since it tried to read from stdin.
`--color off` now disables the CLI progress bar both in the parent process and the build runner process.
Before this fix, the dup2 of the progress pipe was clobbering the cwd fd, causing the fchdir to return ENOTDIR in between fork() and exec().
7281cc1 did not solve the problem because even when Node.index is none, it still counts as initializing the global Progress object. Just use a normal zig optional, and all is good.
Generates better machine code, particularly on ARM
Fixed up and rebased my Windows console API stuff on top of the latest commits here: squeek502@5b49900 (note: it's a different branch than the one in my previous comment) Feel free to cherry pick the commit, or use it as reference and adapt it however you'd like. |
Hmm, the windows implementation has the undesirable property that it clears the screen, including all scrollback. Ideally it would not do that. In fact I think it's better to show no progress than to destroy scrollback history. Maybe I named that function poorly - it does not clear the full terminal, it only clears the progress data that has been written. It's also not good to do the clear inside I think it would be better to spawn an entirely different update thread function for the case of windows console API. |
* Merge a bunch of related state together into TerminalMode. Windows sometimes follows the same path as posix via ansi_escape_codes, sometimes not. * Use a different thread entry point for Windows API but share the same entry point on Windows when the terminal is in ansi_escape_codes mode. * Only clear the terminal when the stderr lock is held. * Don't try to clear the terminal when nothing has been written yet. * Don't try to clear the terminal in IPC mode. * Fix size detection logic bug under error conditions.
Might be an artifact from using wine. Haven't tried on my Windows laptop yet |
Can't reproduce this behavior on Windows. Not sure what Wine would be doing for this to be the behavior there, either, since the "clear" is just In testing this, though, I have noticed that what I said in #15206 (comment) / #18692 (comment) appears to be wrong. With a const std = @import("std");
const windows = std.os.windows;
pub fn main() void {
const stderr = std.io.getStdErr();
var mode: windows.DWORD = undefined;
std.debug.assert(std.os.windows.kernel32.GetConsoleMode(stderr.handle, &mode) != 0);
const new_mode = mode ^ windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING;
std.debug.assert(std.os.windows.kernel32.SetConsoleMode(stderr.handle, new_mode) != 0);
const now_enabled = new_mode & windows.ENABLE_VIRTUAL_TERMINAL_PROCESSING != 0;
std.debug.print("{s} virtual terminal processing for stderr: {}\n", .{ if (now_enabled) "enabled" else "disabled", stderr.handle });
} then running it in Windows Terminal results in:
(that is, the terminal is resetting the console mode to the default for each run; the mode is not preserved between runs which was what I thought happened [and is what happens with the code page]) With
So another option here might be to revisit those PRs (I like #18692 best) and try setting |
* origin/main: (88 commits) document -Drandom Update .woodpecker/eowyn.yaml Rename 'std.rand' to 'std.Random' attempt at implementing #113 "Add a way to do one random exercise" Add patch. 046: Show usage of `.?` and hint towards new solution. Fixes because of a new Zig version, which changes some functions. Calling `split` is deprecated English fixes for 106_files.zig English fixes for 107_files2.zig fixed typo New Zig version Ignore .zig-cache for new zig versions Fixed the changes from reworking std.Progress. For details: ziglang/zig#20059 Verbs agree with the head of the noun phrase, not the closest noun the Earth Update exercises/105_threading2.zig Nobody wants the long version of finding out if a variable is set. So switched to the short version with 'orelse'. ;) Add devcontainer.json Fix some typos ...
* Fix zig_exe location in Build struct Reflect Zig breaking changes as of ziglang/zig@105db13 * Text improvement closes #47 * Zig version adjusted. * Added notes to exercise 94 c_math. * Added threading exercise * Fixed unicode literal * Minor corrections to the "contributing" * fix exercise 82 output zig commit bd24e66 changed the floating point formatting implementation so output for exercise 82 no longer matched * Improved the explanation about passing arguments and added an example. * Changes for a new Zig version. * Fixed the renaming of std.os to std.posix * Added second threading exercise. * Update exercises/105_threading2.zig Fixed typo. * Update .woodpecker/eowyn.yml fix warning * Update .woodpecker/eowyn.yml * Update .woodpecker/eowyn.yml * added exercise/106_files.zig * modified build.zig * 106_files.zig format * add patch files for 106_files.zig * 106_files.zig actual test * 106_files.patches actual * remove header of patch files of 106 * specify directory on patch file 106 * specify directory on patch file 106 * Pass CI test locally * 106 & 107 * added format parameter {d} * fix typo * Fix breaking zig change to @fieldParentPtr parameters See ziglang/zig#19470 * Zig version changed * Update README.md * Greater gradation of timers built into the threads * Additional timer in thread start added * Update .woodpecker/eowyn.yml * Fixed woodpecker warnings * fixing little typo on exercise 107_files2.zig * Switch to new zig dev release 0.13 * Update .woodpecker/eowyn.yaml Add tag "latest" * Update .woodpecker/eowyn.yaml Add pull command * fix: typo: % instead of @ for a builtin function * fix: typo: removed extra s * fix: many grammatical errors * fix: some grammatical errors * Fix patches for 106 and 107 * Fix some typos * Add devcontainer.json * Nobody wants the long version of finding out if a variable is set. So switched to the short version with 'orelse'. ;) * Update exercises/105_threading2.zig The last word, '"diggits" was misspelled. * the Earth * Verbs agree with the head of the noun phrase, not the closest noun the result...are passed→the result...is passed the number...vary→the number...varies * Fixed the changes from reworking std.Progress. For details: ziglang/zig#20059 * Ignore .zig-cache for new zig versions zig cache directory was updated in ziglang/zig#20115 * New Zig version * fixed typo * English fixes for 107_files2.zig * English fixes for 106_files.zig * Calling `split` is deprecated The `split` function in std mem is depreacted and a `@compileError`, splitSequence, splitAny, or splitScalar should be used instead. * Fixes because of a new Zig version, which changes some functions. * 046: Show usage of `.?` and hint towards new solution. * Add patch. * attempt at implementing #113 "Add a way to do one random exercise" * Rename 'std.rand' to 'std.Random' * Update .woodpecker/eowyn.yaml Add pull request * document -Drandom * fix build files broken by latest 0.14.0-dev changes to Build.Step.MakeOptions * fix tests build file broken by addRemoveDirTree now requiring LazyPath * Fixed error message through a TAB in the comment, see ziglang/zig#20928 * Changed needed zig version. * Add build parameter to start at a specific exercise * Clarification in description for ranges in loops. * Simplified de-referencing for clarification * Added license to contributing * Fixes changes in zig build system. * Insert wrong version number into build file. * Fixes several changes in std.builtin.zig * fixed typo: the date in version changes * Reseted the simplification for de-referencing. * Update LICENSE * Corrects the description of the new @typeinfo().@"struct".fields * New patch file for 82 * Update README.md Added new todo for core lang * 108: Add a exercise for a labeled switch * Add .patch for 108_labeled_switch * update zig version in build and readme * update labeled switch to also have a break statement * downgrade zig version to eowyn version It is just 2 days behind so all features are already present for the new labeled switch test * improve explanantions in labeled switch * update108 .patch due to line change * Minor improvements. * Added Dave as initiator of Ziglings. * Deleted .devcontainer because ist was a test. * Rephrase instruction for clarity * Fixed patch from quiz7 after text changes #Please enter the commit message for your changes. Lines starting * reuse fields * zero * patch * Improved maximumNarcissism * created a new exercise about vectors in zig, gave it number 109 * removed commented solution lines in vectors exercise, added ??? into lines instead * added patch file for 109_vectors * line ending format patch attempt * Fixed formating, created patch file. * Added SIMD. * Fixed link to format source code. * Added missing space after a sentence. * Minor case-related changes. * Deleted unnecessary pointer. * Added patch file. * Updated credits * Updated credits * Update: 108 Labeled switch example to contain default case for exhaustion Update example text to give clarity on default/exhaustive case. Reasoning: The input for the example will not compile if user would want to test this for the logic of a labeled switch. Due the input not being an exhaustive input but rather "any u8 integer" (for the lack of better terminology) we need to use the else branch to indicate that the default case is handled, in this case by just emulating the '4' branch, but this could return an error.InvalidCaseProvided for example. Signed-off-by: mikkurogue <michael.lindemans@outlook.com> * Fix typo vaild → valid * Update: Remove the 4th branch in favour of just making that the else branch Probably easier to not have an "unused" branch that returns the same as the else branch in the example. Signed-off-by: mikkurogue <mikkurogue@noreply.codeberg.org> * Use print alias in exercise 100_for4.zig * Fix patch after print alias fix in 100_fo4.zig * Fix a typo: we need do we need to -> we need to Morning delirium does not help when looking at lots of words * Update exercises/108_labeled_switch.zig The sentence was slightly unclear Signed-off-by: chrboesch <chrboesch@noreply.codeberg.org> * Update exercises/108_labeled_switch.zig Fixed an error in the debug statement and made the text a bit more coherent. Signed-off-by: chrboesch <chrboesch@noreply.codeberg.org> * Suggesting a third exercise for bit manipulation * changed the order of the sections to improve flow * rearranged order of expected output in build.zig * added newline between toggle and set sections to make the expected output match * reset and completed exercise tracking wired in * fmt * tracking, skipping and reset all wired in * cleanup for PR * cleanup for PR * cleanup for PR * cleanup for PR * format fix * added patch file for exercise 110 * Skip 74, the compiler corrects this now. * Added deletion of progress.txt for eowyn * Added delte_progress additional to the end of eowyn * Fix repo link * converted 110 to a quiz (quiz 9) * moved explanatory content below the broken code in main so that the exercise functions more like a quiz * made some simple changes to the wording to reflect the fact that this is a quiz * edited the first two paragraphs * added blank lines between sections to make them easier to find * added header for quiz problems * fixed incorrect bitmask in xor example * fixed minor spelling and grammar typos * fixed formatting * fixed spelling of 'bitmask' * Shuttle weight fixed * Output from #60 adapted * fix: exercises/001_hello.zig (#1) * fix: exercises/002_std.zig (#2) * Exercises/003 assignment (#3) * fix: exercises/003_assignment.zig - assign new constant * fix: exercises/003_assignment.zig - change type of pi to u32 * fix: exercises/003_assignment.zig - change type of negative_eleven to i8 * doc: add HOWTO_REBASE_ORIGINAL_REPO.md (#4) --------- Signed-off-by: mikkurogue <michael.lindemans@outlook.com> Signed-off-by: mikkurogue <mikkurogue@noreply.codeberg.org> Signed-off-by: chrboesch <chrboesch@noreply.codeberg.org> Co-authored-by: Alexander Saltanov <asd@mokote.com> Co-authored-by: Chris Boesch <chrboesch@noreply.codeberg.org> Co-authored-by: dolichomps <dolichomps@noreply.codeberg.org> Co-authored-by: Dizzyi <deeralan827@gmail.com> Co-authored-by: kamidev <1655+kamidev@users.noreply.github.com> Co-authored-by: susubub <susubub@proton.me> Co-authored-by: David Hain <d.hain@gmx.at> Co-authored-by: Rafael Áquila <rafaelaquilaah@hotmail.com> Co-authored-by: rpm0372 <rpm0372@noreply.codeberg.org> Co-authored-by: Roman Frołow <rofrol@gmail.com> Co-authored-by: hippietrail <hippietrail@noreply.codeberg.org> Co-authored-by: Nico Elbers <nico.b.elbers@gmail.com> Co-authored-by: Andrew Dunbar <hippytrail@gmail.com> Co-authored-by: Sebastian <serunmal@gmail.com> Co-authored-by: Alex McHugh <alexmchughnz@icloud.com> Co-authored-by: bsubei <6508762+bsubei@users.noreply.github.com> Co-authored-by: Michael Cooper <mythmon@gmail.com> Co-authored-by: Seeingu <seeingasu@gmail.com> Co-authored-by: Nuno Mendes <98030270+nm-remarkable@users.noreply.github.com> Co-authored-by: factormystic <factormystic@gmail.com> Co-authored-by: bgthompson <benjamin.btstudios@pm.me> Co-authored-by: innerviewer <innerviewerr@gmail.com> Co-authored-by: innerviewer <innerviewer@noreply.codeberg.org> Co-authored-by: mikkurogue <michael.lindemans@outlook.com> Co-authored-by: David Pape <zyzzyxdonta@posteo.de> Co-authored-by: mikkurogue <mikkurogue@noreply.codeberg.org> Co-authored-by: Zorgatone <zorgatone@gmail.com> Co-authored-by: Alexander Sisco <36649949+devspeare@users.noreply.github.com> Co-authored-by: Zendril <kenneth.s.brooks@gmail.com> Co-authored-by: Alexander Sisco <alexsisco@noreply.codeberg.org>
Demo
simple asciinema demo source
demo: building a music player with zig build source
Motivation
The previous implementation of
std.Progress
had the design limitation that it could not assume ownership of the terminal. This meant that it had to play nicely with sub-processes purely via what was printed to the terminal, and it had to play nicely with progress-unaware stderr writes to the terminal. It also was forbidden from installing a SIGWINCH handler, or running ioctl to find out the rows and cols of the terminal.This implementation is designed around the idea that a single process will be the sole owner of the terminal, and all other progress reports will be communicated back to that process. With this change in the requirements, it becomes possible to make a much more useful progress bar.
Strategy
This creates a standard "Zig Progress Protocol" and uses it so that the same
std.Progress
API works both when an application is the main owner of a terminal, and when an application is a child process. In the latter case, progress information is communicated semantically over a pipe to the parent process.The file descriptor is given in the
ZIG_PROGRESS
environment variable.std.process.Child
integrates with this, so attaching a child's progress subtree in a parent process is as easy as setting thechild.progress_node
field before callingspawn
.In order to avoid performance penalty for using this API, the
Node.start
andNode.end
APIs are thread-safe, lock-free, infallible, and do minimal amount of memory loads and stores. In order to accomplish this, a statically allocated buffer ofNode
storage is used - one array for parents, and one array for the rest of the data. Children are not stored. The statically allocated buffer is used for a bespokeNode
allocator implementation. A static buffer is sufficient because we can set an upper bound on supported terminal width and height. If the terminal size exceeds this, the progress bar output will be truncated regardless.A separate thread periodically refreshes the terminal on a timer. This progress update thread iterates over the entire preallocated parents array, looking for used nodes. This is efficient because the parents array is only 200 8-bit integers, or about 4 cache lines. When iterating, this thread "serializes" the data into a separate preallocated array by atomically loading from the shared data into data that is only touched by a single thread - the progress update thread. It then looks for nodes that are marked with a file descriptor that is a pipe to a child process. Such nodes are replaced during the serialization process with the data from reading from the pipe. The data can be memcpy'd into place except for the parents array which needs to be relocated. Once this serialization process is complete, there are two paths, one for a child process, and one for the root process that owns the terminal.
The root process that owns the terminal scans the serialized data, computing children and sibling pointers. The canonical data only stores parents, so this is where the tree structure is computed. Then the tree is walked, appending to a static buffer that will be sent to the terminal with a single write() syscall. During this process, the detected rows and cols of the terminal are respected. If the user resizes the terminal, it will cause a SIGWINCH which signals the update thread to wake up and redraw with the new rows and cols.
A child process, instead of drawing to the terminal, takes the same serialized data and sends it across a pipe. The pipe is in non-blocking mode, so if it fills up, the child drops the message; a future update will contain the new progress information. Likewise when the parent reads from the pipe, it discards all messages in the buffer except for the last one. If there are no messages in the pipe, the parent uses the data from the last update.
Performance Data Points
Building the self-hosted compiler as a sub-process. This measures the cost of the new API implementations, primarily
Node.start
,Node.end
, and the disappearance ofNode.activate
. In this case, standard error is not a terminal, and thus an update thread is never spawned.Building the self-hosted compiler, with
time zig build-exe -fno-emit-bin ...
so that the progress is updating the terminal on a regular interval.Building my music player application with
zig build
, with the project-local cache cleared. This displays a lot of progress information to the terminal. This data point accounts for many sub processes sending progress information over a pipe to the parent process for aggregation.Upgrade Guide
std.Progress.Node
instead of*std.Progress.Node
(no longer a pointer).node.activate()
. Those are not needed anymore.Node.start
since the data will be copied.std.Progress
more than once. Do that in main() and nowhere else.std.debug.lockStdErr
andstd.debug.unlockStdErr
before writing to stderr to integrate properly withstd.Progress
(std.debug.print
already does this for you).All the options to
start
are optional.Finally, when spawning a child process, populate the
progress_node
field first:Follow Up Work
closes #9633
closes #17821