Skip to content

Scrolling performance regression using intermediate NSImage #796

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

Closed
comex opened this issue Dec 5, 2018 · 15 comments · Fixed by #858
Closed

Scrolling performance regression using intermediate NSImage #796

comex opened this issue Dec 5, 2018 · 15 comments · Fixed by #858
Labels
10.14 Mojave Renderer Text rendering issues, including CoreText renderer
Milestone

Comments

@comex
Copy link

comex commented Dec 5, 2018

I'm filing this issue to track the known scrolling performance regression from #757 (see that thread for more details). Although it's known already, this way I can subscribe to the issue and hopefully get notified when it's fixed. :)

ychin added a commit to ychin/macvim that referenced this issue Dec 5, 2018
Vim patch 8.1.560

Targets macOS 10.8+

Features:
- macOS Mojave (10.14) is now supported.
    - MacVim's UI now works with Dark Mode.
    - Fixed broken rendering and flickering under Mojave when using the
      default Core Text renderer. macvim-dev#757
- guioption 'k' is supported again. macvim-dev#731
    - This option prevents window from resizing when UI elements such as
      toolbars or tabs show or hide themselves.

Fixes:
- Fixed misc fullscreen and window resizing bugs and artifacts macvim-dev#745
- Dragging tabs to reorder now works properly macvim-dev#789
- Fixed timer callback handling in GUI macvim-dev#749
- Fixed native tabs (10.12+) interring with Vim tabs macvim-dev#788
- Fixed Japanese IME Ctrl-U/Ctrl-O handling macvim-dev#742
- Fixed MMShareFindPboard and Cmd-E/Cmd-G interactions macvim-dev#780
- Better handling of guifontwide font size macvim-dev#737
- Better python discovery in default vimrc macvim-dev#739

Known Issues:
- Scrolling performance is slightly worse under Mojave macvim-dev#796

Script interfaces have compatibility with these versions:
- Lua 5.3
- Perl 5.18
- Python2 2.7
- Python3 3.7
- Ruby 2.5
ychin added a commit that referenced this issue Dec 5, 2018
Vim patch 8.1.560

Targets macOS 10.8+

Features:
- macOS Mojave (10.14) is now supported.
    - MacVim's UI now works with Dark Mode.
    - Fixed broken rendering and flickering under Mojave when using the
      default Core Text renderer. #757
- guioption 'k' is supported again. #731
    - This option prevents window from resizing when UI elements such as
      toolbars or tabs show or hide themselves.

Fixes:
- Fixed misc fullscreen and window resizing bugs and artifacts #745
- Dragging tabs to reorder now works properly #789
- Fixed timer callback handling in GUI #749
- Fixed native tabs (10.12+) interring with Vim tabs #788
- Fixed Japanese IME Ctrl-U/Ctrl-O handling #742
- Fixed MMShareFindPboard and Cmd-E/Cmd-G interactions #780
- Better handling of guifontwide font size #737
- Better python discovery in default vimrc #739

Known Issues:
- Scrolling performance is slightly worse under Mojave #796

Script interfaces have compatibility with these versions:
- Lua 5.3
- Perl 5.18
- Python2 2.7
- Python3 3.7
- Ruby 2.5
@ychin ychin added the Renderer Text rendering issues, including CoreText renderer label Dec 12, 2018
@ychin ychin added this to the snapshot-155 milestone Dec 20, 2018
@s4y
Copy link
Contributor

s4y commented Feb 16, 2019

Here's a first pass at a change which significantly improves performance for me. I'm not quite ready to make a PR, but please try it out:

https://github.com/s4y/macvim/tree/stateful-render

All feedback is welcome.

@s4y
Copy link
Contributor

s4y commented Feb 16, 2019

Two notes re. the above:

  1. Some of the new rendering code is just copied and pasted from MMCoreTextView's three other rendering paths. Since it seems to do as well as them, I'd love to delete the other paths in my PR instead of trying to make them all work at the same time.

  2. I'm not done making rendering faster 😉. If this works out, I know what I want to do next.

@amadeus
Copy link

amadeus commented Feb 16, 2019

Omg, this is night and day faster!

I have a very fast keyboard repeat rate and so scroll performance is super noticeable when it chugs!

Great work so far, excited for what's to come!

(I'm on vacation right now - so won't have a ton of opportunity to test)

@juliuszint
Copy link

juliuszint commented Feb 18, 2019

Performance looks great but UltiSnips stopped working with this patch. Otherwise great work!

@ychin
Copy link
Member

ychin commented Feb 18, 2019

@juliuszint How does UltiSnips not work? Please file another issue for that. Also there's a new version coming out that fixes Ctrl-C not working but it's hard to tell if it's related without more info.

@juliuszint
Copy link

@ychin Sorry, that was just meant as a feedback for the patch that @s4y provided. Since he explicitly wrote that this code is not ready for a pull request and not merged into master i think filing a issue would be a little bit early. I never encountered this problem with a official MacVim release.

@ychin
Copy link
Member

ychin commented Feb 18, 2019

@juliuszint Ah ok. Cool good to know. One less bug for me to fix.

@s4y if you feel like it I would recommend making a pull request with "[WIP]" as a prefix to the title so conversations and discussions on your fix can be focused there. It's up to you though.

@ychin ychin modified the milestones: snapshot-156, snapshot-157 Feb 19, 2019
@amadeus
Copy link

amadeus commented Feb 19, 2019

One small thing I have noticed:

example

Every once in a while, a tiny artifact will appear, in roughly the same-ish location. It looks like a dead pixel but I can confirm its not since (a it comes through on screenshots) and be it follows the window when I move it around. If I scroll the document it goes away

s4y added a commit to s4y/macvim that referenced this issue Feb 19, 2019
This change replaces the current rendering paths in `MMCoreTextView`
with one which draws from an array of structs that represent the state
of each character cell. It applies draw commands to the array and marks
any touched cells as needing display instead of drawing to a view or
image buffer.

In my own tests, this renderer seems to be roughly twice as fast of the
current one. Because it seems to work for the use cases of all of the
current renderers and reused a lot of code, this PR replaces the other
paths instead of adding a preference.

Fixes macvim-dev#796.
@s4y
Copy link
Contributor

s4y commented Feb 19, 2019

@ychin et. al: See #858!

s4y added a commit to s4y/macvim that referenced this issue Feb 19, 2019
This change replaces the current rendering paths in `MMCoreTextView`
with one which draws from an array of structs that represent the state
of each character cell. It applies draw commands to the array and marks
any touched cells as needing display instead of drawing to a view or
image buffer.

In my own tests, this renderer seems to be roughly twice as fast of the
current one. Because it seems to work for the use cases of all of the
current renderers and reused a lot of code, this PR replaces the other
paths instead of adding a preference.

Fixes macvim-dev#796.
@s4y
Copy link
Contributor

s4y commented Feb 19, 2019

@amadeus Let's continue on the PR? The current patch might affect this, but might also not.

@juliuszint If there's any more detail you can share (on the PR), please do!

s4y added a commit to s4y/macvim that referenced this issue Mar 23, 2019
This change replaces the current rendering paths in `MMCoreTextView`
with one which draws from an array of structs that represent the state
of each character cell. It applies draw commands to the array and marks
any touched cells as needing display instead of drawing to a view or
image buffer.

In my own tests, this renderer seems to be roughly twice as fast of the
current one. Because it seems to work for the use cases of all of the
current renderers and reused a lot of code, this PR replaces the other
paths instead of adding a preference.

Fixes macvim-dev#796.
s4y added a commit to s4y/macvim that referenced this issue Mar 28, 2019
This change replaces the current rendering paths in `MMCoreTextView`
with one which draws from an array of structs that represent the state
of each character cell. It applies draw commands to the array and marks
any touched cells as needing display instead of drawing to a view or
image buffer.

In my own tests, this renderer seems to be roughly twice as fast of the
current one. Because it seems to work for the use cases of all of the
current renderers and reused a lot of code, this PR replaces the other
paths instead of adding a preference.

Fixes macvim-dev#796.
s4y added a commit to s4y/macvim that referenced this issue May 1, 2019
This change replaces the current rendering paths in `MMCoreTextView`
with one which draws from an array of structs that represent the state
of each character cell. It applies draw commands to the array and marks
any touched cells as needing display instead of drawing to a view or
image buffer.

In my own tests, this renderer seems to be roughly twice as fast of the
current one. Because it seems to work for the use cases of all of the
current renderers and reused a lot of code, this PR replaces the other
paths instead of adding a preference.

Fixes macvim-dev#796.
s4y added a commit to s4y/macvim that referenced this issue May 1, 2019
This change replaces the current rendering paths in `MMCoreTextView`
with one which draws from an array of structs that represent the state
of each character cell. It applies draw commands to the array and marks
any touched cells as needing display instead of drawing to a view or
image buffer.

In my own tests, this renderer seems to be roughly twice as fast of the
current one. Because it seems to work for the use cases of all of the
current renderers and reused a lot of code, this PR replaces the other
paths instead of adding a preference.

Fixes macvim-dev#796.
s4y added a commit to s4y/macvim that referenced this issue May 3, 2019
This change replaces the current rendering paths in `MMCoreTextView`
with one which draws from an array of structs that represent the state
of each character cell. It applies draw commands to the array and marks
any touched cells as needing display instead of drawing to a view or
image buffer.

In my own tests, this renderer seems to be roughly twice as fast of the
current one. Because it seems to work for the use cases of all of the
current renderers and reused a lot of code, this PR replaces the other
paths instead of adding a preference.

Fixes macvim-dev#796.
s4y added a commit to s4y/macvim that referenced this issue May 7, 2019
This change replaces the current rendering paths in `MMCoreTextView`
with one which draws from an array of structs that represent the state
of each character cell. It applies draw commands to the array and marks
any touched cells as needing display instead of drawing to a view or
image buffer.

In my own tests, this renderer seems to be roughly twice as fast of the
current one. Because it seems to work for the use cases of all of the
current renderers and reused a lot of code, this PR replaces the other
paths instead of adding a preference.

Fixes macvim-dev#796.
s4y added a commit to s4y/macvim that referenced this issue May 7, 2019
This change makes `MMCoreTextView` apply draw commands to an array of
structs that represent the state of each character cell. It marks
changed cells as needing display, and drawRect: draws from the array.

In my own tests, this renderer seems to be roughly twice as fast as the
current one. Because it seems to work for the use cases of all of the
current renderers and duplicates a lot of code, this PR replaces the
other paths instead of adding a preference.

Fixes macvim-dev#796.
s4y added a commit to s4y/macvim that referenced this issue May 7, 2019
This change makes `MMCoreTextView` apply draw commands to an array of
structs that represent the state of each character cell. It marks
changed cells as needing display, and drawRect: draws from the array.

In my own tests, this renderer seems to be roughly twice as fast as the
current one. Because it seems to work for the use cases of all of the
current renderers and duplicates a lot of code, this PR replaces the
other paths instead of adding a preference.

Fixes macvim-dev#796.
s4y added a commit to s4y/macvim that referenced this issue May 10, 2019
This change makes `MMCoreTextView` apply draw commands to an array of
structs that represent the state of each character cell. It marks
changed cells as needing display, and drawRect: draws from the array.

In my own tests, this renderer seems to be roughly twice as fast as the
current one. Because it seems to work for the use cases of all of the
current renderers and duplicates a lot of code, this PR replaces the
other paths instead of adding a preference.

Fixes macvim-dev#796.
@nuc
Copy link

nuc commented May 14, 2019

Just in case it's helpful, for me it's also affecting how fast the caret travels.

I have a really high key repeat rate, with really low delay, so the effect is really noticeable.

@pirminis
Copy link

@nuc exactly same here. however, there is an open PR #858 which fixes the issue

@ychin ychin modified the milestones: snapshot-157, snapshot-158 Jul 17, 2019
s4y added a commit to s4y/macvim that referenced this issue Oct 2, 2019
This change makes `MMCoreTextView` apply draw commands to an array of
structs that represent the state of each character cell. It marks
changed cells as needing display, and drawRect: draws from the array.

In my own tests, this renderer seems to be roughly twice as fast as the
current one. Because it seems to work for the use cases of all of the
current renderers and duplicates a lot of code, this PR replaces the
other paths instead of adding a preference.

Fixes macvim-dev#796.
@ychin ychin modified the milestones: snapshot-158, snapshot-159 Oct 16, 2019
@ychin ychin modified the milestones: snapshot-160, snapshot-161 Oct 26, 2019
s4y added a commit to s4y/macvim that referenced this issue Oct 30, 2019
This change makes `MMCoreTextView` apply draw commands to an array of
structs that represent the state of each character cell. It marks
changed cells as needing display, and drawRect: draws from the array.

In my own tests, this renderer seems to be roughly twice as fast as the
current one. Because it seems to work for the use cases of all of the
current renderers and duplicates a lot of code, this PR replaces the
other paths instead of adding a preference.

Fixes macvim-dev#796.
s4y added a commit to s4y/macvim that referenced this issue Jan 13, 2020
This change makes `MMCoreTextView` apply draw commands to an array of
structs that represent the state of each character cell. It marks
changed cells as needing display, and drawRect: draws from the array.

In my own tests, this renderer seems to be roughly twice as fast as the
current one. Because it seems to work for the use cases of all of the
current renderers and duplicates a lot of code, this PR replaces the
other paths instead of adding a preference.

Fixes macvim-dev#796.
@ychin ychin modified the milestones: snapshot-162, snapshot-163 Mar 3, 2020
s4y added a commit to s4y/macvim that referenced this issue Apr 10, 2020
This change makes `MMCoreTextView` apply draw commands to an array of
structs that represent the state of each character cell. It marks
changed cells as needing display, and drawRect: draws from the array.

In my own tests, this renderer seems to be roughly twice as fast as the
current one. Because it seems to work for the use cases of all of the
current renderers and duplicates a lot of code, this PR replaces the
other paths instead of adding a preference.

Fixes macvim-dev#796.
@ychin ychin modified the milestones: snapshot-163, Backlog Apr 11, 2020
s4y added a commit to s4y/macvim that referenced this issue May 6, 2020
This change makes `MMCoreTextView` apply draw commands to an array of
structs that represent the state of each character cell. It marks
changed cells as needing display, and drawRect: draws from the array.

In my own tests, this renderer seems to be roughly twice as fast as the
current one. Because it seems to work for the use cases of all of the
current renderers and duplicates a lot of code, this PR replaces the
other paths instead of adding a preference.

Fixes macvim-dev#796.
@gorkunov
Copy link

Could someone share build here for test. I cant build it locally due to Error 65 which should be resolved by installing Xcode - but it doesn't help me.

@juliuszint
Copy link

MacVim.app.zip
Configured with ./configure --enable-python3interp. Hope this works for you.

@eirnym
Copy link
Contributor

eirnym commented May 24, 2020

MacVim.app.zip
Configured with ./configure --enable-python3interp. Hope this works for you.

Better to make it with python3 dynamic

s4y added a commit to s4y/macvim that referenced this issue Jul 9, 2020
This change makes `MMCoreTextView` apply draw commands to an array of
structs that represent the state of each character cell. It marks
changed cells as needing display, and drawRect: draws from the array.

In my own tests, this renderer seems to be roughly twice as fast as the
current one. Because it seems to work for the use cases of all of the
current renderers and duplicates a lot of code, this PR replaces the
other paths instead of adding a preference.

Fixes macvim-dev#796.
s4y added a commit to s4y/macvim that referenced this issue Sep 20, 2020
This change makes `MMCoreTextView` apply draw commands to an array of
structs that represent the state of each character cell. It marks
changed cells as needing display, and drawRect: draws from the array.

In my own tests, this renderer seems to be roughly twice as fast as the
current one. Because it seems to work for the use cases of all of the
current renderers and duplicates a lot of code, this PR replaces the
other paths instead of adding a preference.

Fixes macvim-dev#796.
s4y added a commit to s4y/macvim that referenced this issue Oct 16, 2020
This change makes `MMCoreTextView` apply draw commands to an array of
structs that represent the state of each character cell. It marks
changed cells as needing display, and drawRect: draws from the array.

In my own tests, this renderer seems to be roughly twice as fast as the
current one. Because it seems to work for the use cases of all of the
current renderers and duplicates a lot of code, this PR replaces the
other paths instead of adding a preference.

Fixes macvim-dev#796.
s4y added a commit to s4y/macvim that referenced this issue Oct 17, 2020
This change reworks `MMCoreTextView` so that draw commands update an
internal data structure that represents the state of the screen, instead
of drawing them directly to the screen or to an intermediate image.
Then, `drawRect:` can draw any part of the view at any time, as needed.

This change came about when the old strategy stopped working. The old
strategy called `drawRect:` for the entire view to handle a draw command
from the backend, and relied on being able to draw only the changes on
top of the old content of the view. This did not work in new versions of
macOS that use layers, because `drawRect:` was now expected to fill the
entire rect with new content and, if it didn't, the rest of the view
would just contain garbage. bbad3ed
worked around this issue by adding an intermediate CGImage which was
preserved between draws. This fixed the problem but made rendering
slower.

With the change, the intermediate image is no longer needed and
rendering is much faster overall, which resolves macvim-dev#796.

As part of this change, font substitution is now handled by Core Text,
which changes which fallback fonts are used in some cases but makes
MacVim consistent with other macOS apps.
s4y added a commit to s4y/macvim that referenced this issue Dec 3, 2020
This change reworks `MMCoreTextView` to keep track of the state of the
screen instead of drawing to the screen or to an image. This lets
`drawRect:` draw any part of the view at any time, as needed.

This change came about when the old strategy stopped working: The old
strategy calls `drawRect:` for the entire view to handle any draw
command from the backend, but drew only the changes on top of the old
content of the view. This did not work in new versions of macOS that use
layers, because `drawRect:` is now expected to fill the entire rect with
new content. If it doesn't, the rest of the view will just contain
garbage. bbad3ed worked around this
issue by adding an intermediate CGImage which was preserved between
draws. This fixed the problem but made rendering slower.

With the change, the intermediate image is no longer needed and
rendering is much faster overall, which resolves macvim-dev#796.

As part of this change, font substitution is now handled by Core Text,
which changes which fallback fonts are used in some cases but matches
other macOS apps.
@ychin ychin closed this as completed in #858 Dec 4, 2020
@ychin ychin modified the milestones: Backlog, snapshot-167 Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.14 Mojave Renderer Text rendering issues, including CoreText renderer
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants