Render lines via tiles #6733
Render lines via tiles #6733
Conversation
Except that now we store the scrolling tile, instead of the scrolling row.
This will prevent, for example, a mini editor (which has no `@height` by default) from not showing characters.
We did so by introducing an `absolute` (optional) parameter in `pixelPositionForScreenPosition`.
| @@ -0,0 +1,41 @@ | |||
| module.exports = | |||
| class LinesPresenter | |||
There was a problem hiding this comment.
This should really be TilePresenter.
|
✨ Really exciting work. |
| @@ -963,7 +963,7 @@ class TextEditorPresenter | |||
| hasPixelPositionRequirements: -> | |||
| @lineHeight? and @baseCharacterWidth? | |||
|
|
|||
| pixelPositionForScreenPosition: (screenPosition, clip=true) -> | |||
| pixelPositionForScreenPosition: (screenPosition, clip=true, {absolute}={}) -> | |||
There was a problem hiding this comment.
Talk to me a little about this absolute flag... seems like you should always just choose the position relative to the viewport now since the lines layer is no longer moving. Either way, I think we should avoid allocating the options object on this hot path. I'm paranoid about garbage objects these days.
|
I've been talking about wanting to do tiles for a year now. Thanks so much for pushing this forward! |
|
Wow, looks like a huge speedup |
|
Always good to see speedups! |
This commit makes the method return a `top` value which is relative to the viewport. However, we still need to return an absolute value for `left` because lines are moved horizontally through a single transform on their parent node. I would prefer being consistent (i.e. make everything relative), but moving every single tile to simulate scrolling, on the other hand, seems overkill and it's not really intuitive given that we are not tiling horizontally. /cc: @nathansobo
|
@nathansobo @as-cii i am testing on linux 64bit. When scrolled to bottom part , it freeze up , taking 100% CPU and seems to be generating tiles, for 20-40 secs? Is it possible to move tiles generation to a worker process? I have tested with 205 and it rendered fast and fine. |
|
Hi @v3ss0n, thank you for your feedback! Could you please attach somewhere the file you're talking about? If it's private and you don't want to share it with everyone, it'd be awesome if you could share it via e-mail (you can find my address on my profile, https://github.com/as-cii). Thanks a lot! |
|
@as-cii thanks a lot for fast response. I restarted atom and reopened, first it stop responding while atom start , and loading the file , so i start from scratch (previously auto reload have a lot of tabs and split panes opened ). Now it rendering fine . I tested without --safe. So only problem remain is when previous session restored , and large files are opened previously atom stop responding and has to force close (can reproduce with --safe ) Rendering is smooth and fine , i copy / paste that file 10 times , making 11mb , and it dosen't slow down scrolling at all! Thanks a lot for hard work , i will upload the file i testing to gist in a bit. |
|
Here are things i've tested so far: ###Works
##Fails
|
|
This looks very interesting nodejs/node#1159 |
|
@alexandernst That looks super interesting. Thanks for the heads up. |
|
@nathansobo does Atom already use io.js or is it still on Node? |
|
@ilanbiala Atom uses io.js – Electron uses it. |
|
@ilanbiala Yep. It could take a while for this feature to get to us however since it's not even merged to master in iojs. Then it has to be released in an official iojs release, integrated into Electron, and finally Atom has to upgrade to that version of Electron. But definitely worth keeping an eye on. It's something we could definitely make lots use of once it's available. |
|
This may be a bit offtopic but curious. |
|
@v3ss0n From what I understand atom has a packaged version. The version you have is only used to bootstrap the process. |
|
@v3ss0n it is currently io v1.6.3 and being updated to io v2.2.1 |
|
ah i got it , it have packaged own IO.js separated from OS version ? |
|
We have a problem with tile rendering , effecting themes with background transparency : https://github.com/v3ss0n/steam-pirate-ui
from styling side , should i theme |
|
Hey, awesome work on the tile rendering! I've got one problem with it, however, as it breaks my block-cursor package. To have a block-cursor, I used to add a (opaque) background color to the cursor element, and set its Now, with the new tile rendering, all the lines are inside a new element with an opaque background, so I can't render the cursor behind the lines anymore (or it would also be behind the tile, and thus invisible anyway)... There's the option to set a very low alpha value on the cursor color, and render it in front of the text, but that way the cursor changes the text color, and it's tricky to find a value that produces a nice color and keeps text legible. |
|
it seems we have no choice but to embrace it ? The benefits outweighs a few styling inflexibility , but i miss my beautiful theme tho . |
|
Hmmm, I just thought that highlights might suffer from the same issue, but then I found that a highlights container is added to each tile. I wonder if this is also possible for the cursors, or would it be too much of a performance issue? Or just too much work for too little gain? |
|
Highlights? the Highlight selected package ? or Editor's highlight? I am gonna give a try. I was too busy these days and haven't hack atom much last 2-3 weeks. |
Given that tiles have an opaque background, I am afraid this is the only possible solution. Moreover, that would be consistent with how we handle highlights, as you’re pointing out. I kinda overlooked it because Atom’s default cursor works just fine as it stands in front of the tiles; the implementation will definitely bring some complexity but, ultimately, I think it’s the right way of doing it and, after all, it won’t mess things up that much. @olmokramer: that said, what do you think about opening a brand new issue where we can keep track of this? I plan to work on it, but I’ll be traveling over the next days and I think I’ll be able to tackle it starting from the next month.
@v3ss0n: highlights are an abstraction useful to decorate markers; you can have a look at the related blog post for all the nitty-gritty details. |
|
Thanks , interesting , i will look into it. |
|
@as-cii Thanks! That's truly awesome :) I'll submit a new issue in the evening. |
This PR introduces a change in how we render lines. It does so by organizing them into tiles, which we then move up and down through a compositing transformation. The main advantage is that paint times dramatically reduce, because now we simply need to paint a small tile, instead of all the lines. This is especially noticeable during scrolling which, as a result, will be the subject of the benchmarks I am going to show later on.
Please, note that:
TilesPresenterandLinesPresenterand they're probably going to change. Any suggestion or improvement is very welcome here.🐎 And now some benchmarks! 🐎
Benchmarks
Tests conducted using:
A
1920x1080windoweditor.scrollSensitivity = 130A custom code (associated to a keyboard key) to simulate
mouseWheeleventsKeyboard Key Repeat:
Fast(Mac OS X setting)Delay Until Repeat:
Short(Mac OS X setting)I needed a (more or less) repeatable way to benchmark scrolling. To try this out, you can simply scroll up and down as you would normally do.
master
as-tiled-rendering
Why this matters?
As you can notice, paint times are dramatically reduced. This may not be noticeable with your 👀, but it's nevertheless important 'cause it leaves some room for additional operations (such as a more expensive character measurement computation). As said earlier, things are going to speed up further once we introduce tiling for line numbers as well.
It would be great if you could try this out, reporting any eventual regression or glitch that I may have overlooked while implementing the tiling strategy. Moreover, any code review or additional 👀 are always welcome.
Thank you!
/cc: @atom/feedback @nathansobo