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

Utf32 buffer #1878

Merged
merged 41 commits into from Apr 2, 2019
Merged

Utf32 buffer #1878

merged 41 commits into from Apr 2, 2019

Conversation

jerch
Copy link
Member

@jerch jerch commented Jan 3, 2019

This PR applies the UTF32 buffer layout as discussed in #791. This layout rearranges the stored information to make room for the upcoming true color support.

Note: The PR is based on #1796 as it relies on changes from there. After #1796 has landed it can be rebased/merged.

Fixes #791.

@jerch jerch added the work-in-progress Do not merge label Jan 3, 2019
@jerch jerch closed this Jan 4, 2019
@jerch jerch reopened this Jan 4, 2019
@jerch
Copy link
Member Author

jerch commented Jan 4, 2019

Some speed numbers (tested with this xterm-benchmark script https://gist.github.com/jerch/07ea5e2dfc0548df63a9113d6d721aee):

  • master
   Context "lib/xterm_perfcases/input.js"
      Context "Terminal: ls -lR /usr/lib"
         Case "#1" : 1 - runtime: 2511.44 ms
         Case "#1" : 1 - throughput: 19.19 MB/s
         Case "#1" : 2 - runtime: 2374.15 ms
         Case "#1" : 2 - throughput: 20.30 MB/s
         Case "#1" : 3 - runtime: 2348.06 ms
         Case "#1" : 3 - throughput: 20.53 MB/s
         Case "#1" : 3 runs - average runtime: 2411.22 ms
         Case "#1" : 3 runs - average throughput: 20.01 MB/s
  • this branch
   Context "lib/xterm_perfcases/input.js"
      Context "Terminal: ls -lR /usr/lib"
         Case "#1" : 1 - runtime: 2088.04 ms
         Case "#1" : 1 - throughput: 23.08 MB/s
         Case "#1" : 2 - runtime: 1857.33 ms
         Case "#1" : 2 - throughput: 25.95 MB/s
         Case "#1" : 3 - runtime: 1835.57 ms
         Case "#1" : 3 - throughput: 26.26 MB/s
         Case "#1" : 3 runs - average runtime: 1926.98 ms
         Case "#1" : 3 runs - average throughput: 25.10 MB/s

@juancampa
Copy link
Contributor

This is great! Both in terms of performance and because it unblocks the coming WebGL renderer from customizing selection color, and of course, true color support :)

@jerch Do you have an ETA for this to be ready for review/merge? I can help with reviews, if you guys need an extra (albeit not as knowledgeable in xterm internals) pair of eyes

@jerch
Copy link
Member Author

jerch commented Jan 10, 2019

@juancampa The active codebase should already work with this. The test cases are still missing, they are not yet ported to the new interface (tedious to get those moved over). I plan to get it ready for review by the weekend.

This is planned for the 3.11 release, but we will have to wait for the reflow PR #1864 and PR #1796 as they have interdependencies. After that the road is clear to work directly on true color and UTF8 support.

@jerch jerch self-assigned this Jan 17, 2019
@jerch jerch added type/enhancement Features or improvements to existing features area/performance labels Jan 17, 2019
@jerch jerch mentioned this pull request Jan 20, 2019
@jerch
Copy link
Member Author

jerch commented Jan 28, 2019

#1796 got merged, thus the change list here got way shorter. Up for a "normal" review against master.

}

/** Get data as CharData. */
public getAsCharData(): CharData {
Copy link
Member

Choose a reason for hiding this comment

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

This is only used in tests so I think we should move it to TestUtils.test.ts (and pass in the ICellData as an arg)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yepp, good idea.

Tyriar
Tyriar previously requested changes Feb 5, 2019
src/renderer/TextRenderLayer.ts Outdated Show resolved Hide resolved
src/BufferLine.ts Outdated Show resolved Hide resolved
src/BufferLine.ts Show resolved Hide resolved
src/BufferLine.ts Outdated Show resolved Hide resolved
src/BufferLine.ts Outdated Show resolved Hide resolved
src/renderer/CharacterJoinerRegistry.ts Outdated Show resolved Hide resolved
src/InputHandler.ts Outdated Show resolved Hide resolved
src/InputHandler.ts Outdated Show resolved Hide resolved
src/InputHandler.ts Outdated Show resolved Hide resolved
src/Buffer.ts Outdated Show resolved Hide resolved
@Tyriar Tyriar added this to the 3.13.0 milestone Apr 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/performance type/enhancement Features or improvements to existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants