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

Use a circular list to manage buffer #422

Merged
merged 17 commits into from
Dec 28, 2016

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Dec 21, 2016

Fixes #361

This is a work in progress, I'm putting the PR out now so the changes are easier to view and so I can get early feedback. Initial testing shows this is a lot faster in processing a large ls -lR as well as being responsive and showing the output as it's going (as opposed to locking up).

Remaining tasks:

  • BUG: Applications that change middle of buffer screw up everything (eg. git log)
  • Make sure vim works
  • Ensure that all changes to the list that would trigger a change in ybase and ydisp are handled
  • Do thorough manual testing and ensure automated tests pass
  • Benchmark against current
  • Replace usages of CircularList.splice with CircularList.shiftElements where possible

@Tyriar Tyriar self-assigned this Dec 21, 2016
@Tyriar Tyriar requested review from akalipetis and parisk December 21, 2016 23:47
@Tyriar
Copy link
Member Author

Tyriar commented Dec 21, 2016

@akalipetis @parisk feel free to give this some early feedback if you have the time.

@parisk parisk added the work-in-progress Do not merge label Dec 22, 2016
@parisk
Copy link
Contributor

parisk commented Dec 22, 2016

Awesome. This looks great (code and functionality-wise).

Giving it a quick try works like a charm 👍 . Also running ls -lR feels by far more responsive.

I witnessed only mess-ups with git log / top:

If I scrolled x pages down in git log and then scrolled y pages up (y<x), some weird flashing happened.

👍 let's move this forward. I'd be happy to help wherever needed.

The old code was assuming that the buffer was not going to change, this is not
true with the current impl though where the list is shifted and ybase and ydisp
need to be compensated for that.
@Tyriar
Copy link
Member Author

Tyriar commented Dec 22, 2016

I still need to do more verification but I believe the bugs are worked out. There are a few lingering TODOs still to do though. After this change we should then get rid of the majority if not all of the CircularList.splice calls and replace them with a much faster function(s) that sets some element and shifts n elements by 1 index as that seems to be its main purpose (see reverseIndex).

An early comparison shows with an ls -lR ~ on my machine gives the following results:

  • master: 43 seconds to finish, browser locked up and showed unresponsive dialog after 30s
  • CircularList: 26 seconds to finish, constant text scrolling ✨

@parisk
Copy link
Contributor

parisk commented Dec 23, 2016

@Tyriar a few comments on the TODO list:

Ensure that all changes to the list that would trigger a change in ybase and ydisp are handled

This will be handled in automated tests, right?

Do thorough manual testing [...]

Can you update this with the test cases you have in mind, in order for the rest of us to contribute as well?

Benchmark against current

Should we consider this as ✔︎ done, since we have the ls -lR ~ metric you provided.

@Tyriar
Copy link
Member Author

Tyriar commented Dec 24, 2016

@parisk

This will be handled in automated tests, right?

If possible, having had jumped into this escape sequence code I have a far better understanding of how it all works. I might follow this one with some refactors and tests, rather than jamming it all into this PR.

Can you update this with the test cases you have in mind, in order for the rest of us to contribute as well?

Ideas right now:

  • See if we can expand the automated escape sequence tests that are already present
  • Play with git, vim, less, etc. when the buffer is full and not full

Should we consider this as ✔︎ done, since we have the ls -lR ~ metric you provided.

I might see if I can write an automated perf test first, it would be nice to have something a little more solid than a timer on my phone 😄

I was also thinking maybe we could look at adding the PR that syncs #146 to see if we've overcome the issues that made this slow. It would be awesome if time was accurate, both for the end user and for testing purposes. Created #425

@Tyriar
Copy link
Member Author

Tyriar commented Dec 24, 2016

Automated testing is probably best left for better support within xterm.js. I did some more tests and compared it to gnome-terminal too:

master CircularList gnome-terminal
Print 1..500000 55s 8s 3.5s
ls -lR ~ (524582 files) 68s 29s 6s

Here's the print test: for x in {1..500000}; do echo $x; done

@Tyriar
Copy link
Member Author

Tyriar commented Dec 24, 2016

This is ready for a full review now.

@parisk parisk removed the work-in-progress Do not merge label Dec 27, 2016
Copy link
Contributor

@parisk parisk left a comment

Choose a reason for hiding this comment

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

👍 This looks mostly 👌. Just a couple of comments and we are good to go.

Also did a few manual tests on this and the experience is definitely faster than before.

@@ -0,0 +1,186 @@
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update the license according to the paradigm set in #388

Copy link
Member Author

Choose a reason for hiding this comment

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


// TODO: Why is this done twice?
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the issue here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's done just below, it could probably be merged with the one below.

@Tyriar Tyriar merged commit e6ad8d8 into xtermjs:master Dec 28, 2016
@Tyriar Tyriar deleted the 361_circular_list_scrollback branch December 28, 2016 11:10
@imsnif imsnif mentioned this pull request Dec 28, 2016
Tyriar added a commit to microsoft/vscode that referenced this pull request Dec 28, 2016
The terminal should be a lot faster at processing output now thanks to
xtermjs/xterm.js#422

Also of note: xtermjs/xterm.js#417: changed escape sequence for alt-arrow to work on bash on os x
@Tyriar Tyriar modified the milestone: 2.3.0 Feb 2, 2017
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.

2 participants