Skip to content
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

WIP: dotsv2: improvements to reduce text jitteriness #353

Closed
wants to merge 12 commits into from

Conversation

howardjohn
Copy link
Contributor

@howardjohn howardjohn commented Aug 9, 2023

For #352

I don't think this is fully ready but its mostly usable as a POC.

Basically the jitteriness comes from two causes:

  1. Unlimited height. If we have 1000 packages, for example, and 50 line terminal then we are still trying to write all 1000 packages. This makes it hard for the terminal emulator to keep up
  2. Fast writes. We write every time an event comes in, which can be very very often. Like 1000s per second. Again, we cannot keep up.

This PR fixes this by only including the last <height> lines (determined from inspecting the terminal) and writing updates on a 100ms timer instead of fixed.

This is inspired by https://github.com/docker/compose/blob/80856eacafcf96d6f27e74c07e42386fe662f8ef/pkg/progress/tty.go#L50 which has, IMO, a pretty nice progress UI. Note they don't do the height part.

Some things I think would make this better/acceptable to merge:

  • The implementation is racy, easy to fix though.
  • We could optimize writes to not write if there are no changes.
    • We could optimize to write faster than every 100ms a few times with backoff to 100ms - probably overkill
  • We can not bother writing >height lines, since we just throw it away anyways
  • We could move to a pull based model, where every 100ms we compute what to write.
  • Prioritize what we write. The last N is not ideal -- we could have [1 running test, 100 skipped tests, 1 running test]. The 2 running test lines are most important. As long as NumCPUs < terminal height, this would always ensure we can see all running tests.

Opening for early feedback

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for tracking down these problems and working on the fix! I think the problems you've found definitely explain the bug.

To throttle the output, could we have dotwriter.Writer.Flush only write every 100ms? If it is called more often we simply resetting the buffer (no print or call to clearLines). That could potentially delay output for longer if a spike of calls to flush is followed by a long delay, but I think most of the time that wouldn't be a problem. We could handle that with a ticker that calls Flush, which I guess would require copying the buffer instead of simply resetting it. I suspect moving the ticker and the locking to dotwriter.Writer may simplify things a bit.

Some additional buffering using bufio also sounds good.

Limiting line height by dropping the lines that won't be visible seems good to me. That may also be easier to do in dotwriter.Writer, I'm not sure.

@dnephin
Copy link
Member

dnephin commented Sep 16, 2023

I assume this was resolved by #368, but if not I'm happy to re-open.

@dnephin dnephin closed this Sep 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants