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

Scrollback #1147

Merged
merged 92 commits into from
Sep 17, 2018
Merged

Scrollback #1147

merged 92 commits into from
Sep 17, 2018

Conversation

jwilm
Copy link
Contributor

@jwilm jwilm commented Mar 9, 2018

Warning: this is under development, is missing several features, and has a number of known bugs. The progress of this feature is being tracked under the Scrollback project which captures all known issues and missing functionality (to be populated after filing this PR).

Motivation

A while ago I mentioned that I had an alternative implementation of scrollback that I was working on, and here it is! First of all, some explanation of why an alternative implementation was needed in the first place is warranted. There are two main reasons,

  • Performance
  • Maintainability

Regarding performance, I've written a small tool, vtebench, for testing certain performance characteristics of terminals that have a big impact on users. From the original scrollback thread, there were some benchmarks recorded which suffered under that implementation. Without diving into details, they are all resolved in this branch. To see for yourself, try running

time vtebench -h $(tput lines) -w $(tput cols) -b 10000000 scrolling-in-region

on master, this PR, and the original scrollback PR. There are some other benchmarks built into the tool that can be seen with vtebench --help. scrolling-in-region just happens to show the biggest disparity.

On the topic of maintainability, this is from the perspective of a maintainer who will be supporting this code for who knows how long into the future. Scrollback fundamentally changes how storage is implemented in the terminal, how updates work, and introduces the need for transformations between "visible screen space" and "storage space" or "buffer space". To help ensure we can continue to grow the project, I want to be intimately familiar with these parts of the code base.

The work @neon64 did was excellent, and the number of people I've seen on the internet claiming they use that branch every day is a testament to that. I am certainly not saying their code is unmaintainable; it's just not the way I thought about the problem, and I didn't know how to think about the problem until playing around with several approaches myself.

I am very thankful for all the work @neon64 did, and I intend to recognize that work in a number of ways:

  1. The bounty for scrollback should be awarded to them after this PR lands. The only reason they haven't already received it is because I've been playing around with this alternative implementation instead of landing theirs
  2. Add a Special Thanks section in several highly visible places. This will be a way to recognize not only @neon64 for this work but several other contributors who have had an outsized impact on the project.
    • The project README
    • The Alacritty website once it launches (does not yet exist)
    • The --help text
    • The man page.

Approach

The approach here starts before ever considering scrollback. There were some improvements to be made in the Grid implementation along with cleanup, and we needed a storage layer which could support fast scrolling operations.

The new storage layer is still based on a Vec, but it's been abstracted behind a struct Storage<T> which only allows certain operations on the Vec. Critically, it's been optimized to make scrolling a simple addition or subtraction to an offset. In this way, the Vec became a circular buffer, and scrolling becomes very cheap.

There's also a new optimization with scrolling regions. These regions are a VT feature which fix certain lines in place while allowing others to scroll; classic examples of this are the colored bar at the bottom of a tmux screen, or the status bar at the bottom of vim. The optimization here is that we can still implement scrolling as a rotation, and then we just need to swap the non-"scrolling" lines back into their correct location.

The scrolling region optimization was inspired by a request on #1000. @maximbaz was asking for the ability to "use tmux and native scrolling" at the same time such that a maximized tmux pane would simply have output move into the native buffer. There's a gif in the ticket for additional clarity. The nice thing about this feature is it actually leads to a very efficient implementation!

In addition to all of the tickets in the project linked at the top, there's a bit more work to do around finalizing transformations between "screen" and "buffer" space. I realized that this could be cleaned up significantly while updating selections to work with scrolling.

Finally, one really important piece to me was that the Term implementation doesn't have to think about conversions to/from buffer coordinates. That should all be handled inside Grid; said another way, Grid should continue to be indexable by Line and Column and do a transformation internally for those cases. This keeps the Term impl much simpler.

Benchmarks

It's getting late and I still need to file tickets for outstanding issues here, but I just want to add that this branch is, for scrolling operations, about 30% faster than master is today in my testing.

Closes #124
Closes #657
Closes #1000
Resolves #836
Resolves #885
Resolves #837
Resolves #923
Resolves #1022

@jwilm
Copy link
Contributor Author

jwilm commented Mar 9, 2018

I've added all of the issues I can recall from memory to the Scrollback project, but I won't claim it's an exhaustive list. 😛

Any PRs related to issues on this project should be directed at the scrollback branch instead of the master branch.

@chrisduerr chrisduerr mentioned this pull request Mar 9, 2018
5 tasks
@chrisduerr
Copy link
Member

I think having LTO for release builds should be fine, especially if we provide binaries so the compilation cost is on us. However if there is no performance improvement measurable at all, leaving it in would probably just be a waste of time.

The scroll_history in the config does not tolerate failure and just falls back to the default config instead of the default value, I've created #1156 for tracking this.

@jwilm
Copy link
Contributor Author

jwilm commented Mar 13, 2018

Need to rebase this on master after landing #1057, but I got stuck trying to figure out how to handle some changes related to TermMode checks. They used to be in scroll_terminal, but now they moved out and there is an early return.

@chrisduerr given that you've look at that code a lot recently, do you have a suggestion on the right approach there? Early return is happening at the top of on_mouse_wheel, but scroll_terminal is used in several places.

@chrisduerr
Copy link
Member

If you're talking about what I think you're talking about, the modes are just not passed anymore, but still checked inside of scroll_terminal. There was no reason to ever pass the modes because they should have been fixed, that's why I removed that parameter and declared the modes inside the scroll_terminal method directly.

https://github.com/jwilm/alacritty/blob/master/src/input.rs#L478-L481

@jwilm
Copy link
Contributor Author

jwilm commented Mar 13, 2018

@chrisduerr

In that case, I think we can just delete the early return from on_mouse_wheel and things should still work properly. Thanks!

@chrisduerr
Copy link
Member

Yeah, the scroll_terminal method should be the one to decide what has to be done, an early return would lead to alacritty not scrolling, or faux scrolling, unless mouse modes are enabled. Modes should also not be passed to scroll_terminal but just defined in there, because we don't want them variable but fixed.

@chrisduerr
Copy link
Member

This PR also fixes #836 and it fixed #885 on my machine (urxvt and alacritty are now even).

@ghost
Copy link

ghost commented Apr 2, 2018

just tested this branch and works fine on first run, one thing I noticed though is: I set alias ce="clear && printf '\e[3J'" which for some reason I don't know but it works, clears my terminal and makes it so that one can not scroll into the "old" region.
I find this extremely helpful for e.g. separating old debug output from new debug output.
This worked in every terminal so far.

@klautcomputing
Copy link

I just wanted to thank everyone who worked on this! Not having scroll back is the single thing that stopped me from using alacritty! 🍠

jwilm added 13 commits June 2, 2018 09:24
VecDeque offers improved performance beyond a plain Vec for common
scrolling situations (full screen scroll). Additionally, VecDeque is
necessary for performant scrollback since recycling old rows for a Vec
would be expensive (push/pop front would shift entire vec).
This is part of some cleanup for the grid module as a whole.
The type selection::Region was defined identially to std::ops::Range.
Using something other than range just served to confuse.
In addition to a marginal performance improvement, this simplifies some
logic in the Term implementation since now the Grid fully handles row
recycling.
This intends to optimize the case where the top of the scrolling region
is the top of the screen. In theory, scrolling in this case can be
optimized to shifting the start/end of the visible region, and then
rearranging any lines that were not supposed to be scrolled (at the
bottom of the region). However, this didn't produce quite the speedup I
expected.
Things that do not work

- Limiting how far back in the buffer it's possible to scroll
- Selections (need to transform to buffer offsets)
@Clyybber
Copy link

Clyybber commented Jul 29, 2018

Nice work!
I guess this is ready to merge?

This has been disabled temporarily to improve compile times, however
there were some performance regressions caused by this change.

Since this only affects release compiles and performance is a high
priority for Alacritty, LTO has been enabled again.
@saulshanabrook
Copy link

I am running this now on Mac and it's working great! Thank you! Is there anyway to clear the scrollback buffer currently? I think this is bound to CMD-K by default in iterm.

@chrisduerr
Copy link
Member

@saulshanabrook Currently this is not possible, however that would be fairly easy to implement if a feature like that is desired. Since Alacritty already supports updating the size of the scrollback history through the configuration file without restarting Alacritty (which basically clears the history).

Please open a separate issue if you would like to see this feature so it can be tracked properly through the github issue tracker.

@saulshanabrook
Copy link

@chrisduerr Sounds good #1480

Copy link
Member

@chrisduerr chrisduerr left a comment

Choose a reason for hiding this comment

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

I've checked the complete diff for potential regressions caused by a merge/rebase and everything looks clean.

There also were no obvious mistakes I was able to find or blocks of TODO comments/commented-out code, so the state of this PR is looking good to me.

ArniDagur and others added 4 commits July 29, 2018 13:41
Since scrollback is implemented on the scrollback branch,
this section in the FAQ is redundant.
When running bash and executing `echo -ne '\033c'`, the terminal should
be cleared. However there was an issue with the visible area not being
cleared, so all the text previously printed would still remain visible.

To fix this, whenever a `reset` call is received now, the complete
visible area is reset to `Cell::default()` (the default Cell) and the
length of the available scrollback history is reset to `0`, which
results in the scrollback history being cleared from the perspective of
the user.

This fixes #1483.
This fixes an `illegal hardware instruction (core dumped)`
error when building in release mode.
Since Alacritty never had any scrollback history, the behavior when the
window height was increased was to just keep the prompt on the same line
it has been before the resize. However the usual behavior of terminal
emulators is to keep the distance from the prompt to the bottom of the
screen consistent whenever possible.

This fixes this behavior by loading lines from the scrollback buffer
when the window height is increased. This is only done when scrollback
is available, so there are only N lines available, the maximum amount of
lines which will be loaded when growing the height is N. Since the
number of lines available in the alternate screen buffer is 0, it still
behaves the same way it did before this patch.

Different terminal emulators have different behaviors when this is done
in the alt screen buffer, XTerm for example loads history from the
normal screen buffer when growing the height of the window from the
alternate screen buffer. Since this seems wrong (the alt screen is not
supposed to have any scrollback), the behavior of Termite (VTE) has been
chosen instead.

In Termite the alt screen buffer never loads any scrollback history
itself, however when the terminal height is grown while the alternate
screen is active, the normal screen's scrollback history lines are
loaded.

This fixes #1502.
@MaxBittker
Copy link

I've been running this branch for a while and haven't had problems

@atopia
Copy link

atopia commented Aug 23, 2018

@jwilm, what's holding this PR back? It seems lots of people have been using this branch as a daily driver on Arch Linux for months and this PR is blocking #1403. which would resolve other issues too.

@nixpulvis
Copy link
Contributor

@atopia I'm not 100% sure what the criteria both @jwilm and @chrisduerr are using to decide when this is ready. But there have been a number of small edge cases being fixed still. It's getting better and better, which is exciting, but also not the fastest process.

@chrisduerr
Copy link
Member

The only real issue blocking this branch right now is finding the time to review on merge this branch. Since it's already taken this long, I don't think there's a point in rushing it now.

I do not expect the scrollback branch to stick around unmerged for a long time now though, based on the reports from users, it seems like it's ready for prime time, so it will be included in due time.

nixpulvis and others added 4 commits September 2, 2018 00:30
The clearing the screen for the `ansi::ClearMode::Saved` enum value
has been implemented. This is used to clear all lines which are
currently outside of the visible region but still inside the scrollback
buffer.

The specifications of XTerm indicate that the clearing of saved lines
should only clear the saved lines and not the saved lines plus the
currently visible part of the grid. Applications like `clear` send both
the escape for clearing history plus the escape for clearing history
when requested, so all sources seem to agree here.

To allow both clearing the screen and the saved lines when a key is
pressed the `process_key_bindings` method has been altered so multiple
bindings can be specified. So it is now possible to execute both `^L`
and `ClearHistory` with just a single binding. The
`process_mouse_bindings` method has also been changed for consistency.

To make sure everything works properly a test has been added which
clears the history and then attempts to scroll. Since scrolling is the
only way for a user to check if scrollback is available, this seems like
a nice abstraction to check if there is a scrollback.
The IL escape sequence (CSI Ps L) allows inserting blank, uninitialized
lines. `Ps` is a placeholder for the number of lines that should be
inserted. Before this change Alacritty would crash when a large number
of lines was passed as `Ps` parameter.

The issue was caused whenever the current line of the cursor plus the
lines that should be inserted would leave the bottom of the terminal,
since this makes indexing impossible.

This patch makes sure that the biggest amount of lines inserted does
never exceed the end of the visible region minus the current line of the
curser, which fixes the underflow issue.

This fixes #1515.
* Change deb installation from crates.io to git

There have been a number of issues an PRs opened since
the cargo-deb installation does not work with the latest
version from crates.io.

To help out users until the crates.io version is updated,
the installation instructions have been temporarily
changed to install `cargo-deb` through github.

* Revert cargo-deb install back to use crates.io

Since `cargo-deb` has been updated on crates.io it is now
possible to just install it from crates.io and build Alacritty's
deb without having to rely on github.

* Update dependencies

This fixes an `illegal hardware instruction (core dumped)`
error when building in release mode.

* Remove redundant copy when selecting font_key

* Bump version number to 0.2.0

Since the Scrollback branch introduces some major changes, this bumps
the version number from 0.1.0 to 0.2.0.

The versions of Alacritty have not been updated regularly to this point,
so the scrollback branch is a good point in time to start updating
Alacritty's version on a regular basis.

Further changes to the readme, like dropping the 'alpha' status and
updating it to 'beta' could also be introduced with this branch. This
way there will be a clean cut which updates everything as soon as
scrollback is merged.

Building versions is another thing which would be a good thing to start
reasonably quickly. However starting this on the main branch after
scrollback has been merged seems like a more reliable way to move
forward.

This fixes #1240.

* Add a CHANGELOG file

A CHANGELOG file has been added to offer a bit more transparency over
which features have been changed, added and potentially removed in
Alacritty.

There are various formats available for the CHANGELOG file but the most
common and sensible one seems to be the one defined by
https://keepachangelog.com/en/1.0.0. Following the template proposed by
this it should be possible to create a clear CHANGELOG which makes it
simple for new contributors to figure out exactly which formatting
should be used for it.

Since there have been quite a few changes to Alacritty already, not all
changes have been added to the changelog. However a few entries have
been ported just to give a bit of an example what the format should look
like. This also helps with the 0.2.0 version since it will not be
completely empty in the changelog.

This fixes #1534.

* Update CHANGELOG

This updates the CHANGELOG to include the changes introduced by
43882ad.
@jwilm jwilm merged commit cff58e9 into master Sep 17, 2018
@ti-mo
Copy link

ti-mo commented Sep 17, 2018

🎉 🙏 🎂

@nixpulvis
Copy link
Contributor

🎉 wooo it's here!

@unphased
Copy link

Epic!

@uri
Copy link

uri commented Sep 19, 2018

First off, I'd like to mention my appreciation for all the work various people have put into getting this feature shipped.

But I'm curious what the motivation for this feature was. The goals of this project as stated on the README are performance and simplicity by offloading to a window manager. It seems that scrollback is one of these features that can (and was) offloaded to another tool. I think it's great that a core maintainer has intimate knowledge of this implementation but it ultimately adds more complexity to the project, which could make some future changes much harder to implement.

The only reason I can come up with is wider adoption as I've seen comments stating that the lack of this feature is a deal breaker. Ultimately, I'm not sure if sacrificing simplicity for mass adoption is a sensible goal -- if a tool doesn't work for you, then don't use it.

Are there any benefits to users who use tools like tmux? Will this feature actually make future changes easier and open up new possibilities? Should this be disabled if using tmux?

Thanks again to all the folks involved. I clearly do not have the whole picture and I hope this doesn't come off as disparaging.

@jedahan
Copy link
Contributor

jedahan commented Sep 19, 2018

@uri I'm actually pretty interested in seeing a performance comparison between this and tmux with scrollback.

Do you know if there is a way to use tmux for window management but alacritty's scrollback? Then there is no downside for those who want to use both, if performance is better.

@nixpulvis
Copy link
Contributor

@uri if it helps you see the motivation, for people who don't use tmux normally this makes things much simpler. The added code complexity is a real thing, but in many places some work has been done to hopefully work well with other things that may be added.

Something a lot of people probably want (I know I do) is text reflow on resize, and now that we have scrollback the implementation of that should be more clear (I think).

I would also be curious how thing compare between this feature and tmux.

I can understand your sentiment though, especially since when this feature was originally planned to be added there was talks of making it opt-in compile feature. I personally feel like adding the compile flag isn't a big deal though, since you can effectively turn off scrollback by setting the size to 0.

@chrisduerr
Copy link
Member

When announcing this change on reddit and HN, one of the main things people still requested from Alacritty was an improvement in terminal latency. Since Alacritty definitely aims to achieve excellent performance, striving for optimal latency definitely makes sense. Any additional tool in the pipeline (like tmux), could potentially lead to a worse latency. So by giving users the option to run without tmux, Alacritty actually clears the path to add improvements in other areas.

While this definitely added a few lines to the code, which weren't inherently super complex, it also allowed for a restructuring of the existing architecture. This allowed Alacritty to get some nice performance improvements in the only areas where it still had some troubles competing with URxvt. While this also would have been possible without making scrollback available, that definitely was a great part of the added complexity in this PR.

These are just some advantages of scrollback. There are certainly other things that were nice about this PR and as someone who doesn't use tmux, I definitely value it a lot.

Ultimately, I'm not sure if sacrificing simplicity for mass adoption is a sensible goal

Lastly I'd like to point out the issue with this statement though. I've never been much of an advertiser for my own projects, but ultimately a tool without users is pretty useless. So if a huge number of existing users, plus a huge number of potential users are requesting a feature (which was definitely the case for scrollback!), I think it's worth sacrificing a bit of complexity.

Ultimately the only two problems with complexity are the work it causes, and potential performance issues. If enough users request a feature, that justifies putting time into it and maintaining this. Since it might attract new contributors who could just maintain it themselves! And as mentioned earlier this PR actually improved performance, so it's not much of an issue.

I hope this explanation made some sense to you @uri, it's always welcome to ask questions when you don't understand something. If you have any more, please let me know. I hope my ramblings made at least some sense to you. :)

@unphased
Copy link

unphased commented Sep 20, 2018

I'll also chime in as a heavy tmux user there are still very common situations when I want to not use tmux. For example I want to connect to a new machine and for whatever reason I want it in a separate terminal window. There is more of a mental separation that way.

The question of complexity is also definitely real. I do find a lot of the time I am exploring a new capability of the operating system or filesystem or CLI tools and I want to eliminate the possibility that software which employ systems like pseudoterminals are potentially interfering with their operation. Indeed, when I know the output will not be enormous, I tend to avoid piping to pagers like less too. In these cases spawning a raw terminal that's running a shell and nothing else is the gold standard.

In these cases I want to spawn a new alacritty instance and I want to run a program (be it ssh or whatever else that may be new to me), and I want to be rawdogging it!

In these situations, having no tmux latency and having scrollback always be there to bail you out so you can see what happened is pretty beneficial. This always used to be a reminder that my terminal was "lacking something". Now it's not!

An optional compile flag to rip it out (leaving scrollback in with the default build) would be ideal IMO.

@chrisduerr
Copy link
Member

@unphased Just on the matter of making it a compile-time flag. There would be no benefit to disabling this at compile time over the config setting which is already available. Since the abstractions for the scrollback buffer are already in the code, it would also introduce additional complexity.

Without putting a lot of extra work and code into Alacritty, there also wouldn't be an immediate performance benefit by switching it off at compile time. If you don't like scrollback, I'd recommend just making use of the config setting.

@h-michael h-michael mentioned this pull request Jul 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment