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

fix #21849: Minimap: Show Full Document #74425

Closed
wants to merge 1 commit into from

Conversation

@lxsk
Copy link

lxsk commented May 27, 2019

fix #64459: Minimap click and drag

  • new option "Minimap: Entire Document"
@msftclas

This comment has been minimized.

Copy link

msftclas commented May 27, 2019

CLA assistant check
All CLA requirements met.

@jogibear9988

This comment has been minimized.

Copy link

jogibear9988 commented Sep 14, 2019

@rebornix @alexandrudima
any news about this pull req? it would really increase the usage of the minimap

@lxsk lxsk force-pushed the lxsk:show_full_minimap branch 3 times, most recently from 569a896 to c8987b4 Oct 14, 2019
@lxsk

This comment has been minimized.

Copy link
Author

lxsk commented Oct 14, 2019

updated the PR with a smaller rebased version of the changes, enable "Settings>Editor>Minimap: EntireDocument" to test the changed minimap rendering and mouse dragging

@lxsk lxsk force-pushed the lxsk:show_full_minimap branch 2 times, most recently from e692c27 to 1c8eef6 Oct 14, 2019
@alexdima

This comment has been minimized.

Copy link
Member

alexdima commented Oct 15, 2019

❤️ Thank you! This is implemented in a nice way, and I really like the way the end-result looks like!

But I cannot accept this as is, due to performance issues. checker.ts, which is not even that big of a file, at 2MB and 30k lines, is having serious difficulties with this new option. We need to be able to handle files of millions of lines (think log files)...

Here are some quick measurements I did for checker.ts which is a pretty normal large file (not a huge one).

The initial render on file open is a whooping 402ms:
image

There's also no throttling of createImageBitmap, and we end up calling that thousands of times, as the tokenization progresses through the lines (we could fix this), but even with that fixed, a subsequent typing at the end of the file is 74ms:
image

These times simply blow through our painting time budget.

Here is the minimap on the same file without entireDocument:
The initial render, 10ms:
image

Updating the minimap on keystroke <1ms:
image

I think for large files, the approach to render all the lines in a bitmap and then scale it down to the minimap size is not viable.

I will also give more thought to the problem, the current solution "allocates" at minimum 1px per line in the bitmap, which means sometimes we end up with a very high bitmap, like in this case of 30000px which is then scaled down to the screen height of 1000px, and each subsequent update has to pay the cost of recreating this very large bitmap...

@alexdima

This comment has been minimized.

Copy link
Member

alexdima commented Oct 15, 2019

I've also tried jquery... This one has only 9000 lines and still that means around 10 or more lines per vertical minimap pixel. The downscale looks good, but at 10 lines per pixel it has a lot of loss, look how pressing Enter completely changes the shape of the minimap, as if the entire file has been changed:

TO_UPLOAD

The best is to zoom in on a section and see how lines appear/disappear as they presumably line up with the power of two the downscaling uses

TO_UPLOAD

In fact, this makes me think that at a certain line - vertical pixel ratio we can render random text in there, since the painted minimap holds no "vertical" correlation whatsoever with the file content... There could be features of the code shape that the minimap captures, such as consistent indentation or a maximum respected line height, or perhaps a tremendously large block comment (100 lines in length or so), but otherwise, we could as well render pixels using Math.random()...

I wonder if sampling would be a better way for rendering such large files...

@lxsk

This comment has been minimized.

Copy link
Author

lxsk commented Oct 15, 2019

Yeah I was guessing that the impact for large files is too much, that's why I initially was trying to split up everything in chunks and only update the affected chunks. This way you ended up with almost always changing only one chunk and all the others are just redrawing the images.
But the initial work still stays and the code ended up a bit too complicated.

There are also a lot of missed opportunities in the current implementation, e.g. the image buffer is always as big as the required size (this is good enough for the old behaviour) but with the whole document visible you end up reallocating on every "Enter"-button press. Which could be easily mitigated. And there are other things as well where the already rendered old data is not reused despite the content is the same (or almost).

But I just wanted to try a minimal "invasive" approach.

Some possible mitigations are just rendering every nth line if the effective line height is below 1px, but I guess this could end up with some weird artifacts. But with >2000 lines things are not visible anyway. At those line counts it would require generating mipmaps (which should be no deal for the GPU, but I don't know how to do it from within such an electron app) and drawing those to prevent this "flickering" appearance.

Another idea is to use a line budged which gets updated every frame, so that huge file changes or the initial update does not hit one frame that much.

I have to check the chrome profiler as well, but I am not familiar with it. (normally I am a c++ guy)

@jogibear9988

This comment has been minimized.

Copy link

jogibear9988 commented Oct 15, 2019

How is it done in full Visual Studio? How does it work with huge files?

@lxsk

This comment has been minimized.

Copy link
Author

lxsk commented Oct 15, 2019

@alexandrudima I have added an undersampling (so it skips lines on large files) this helps a lot, but the tokenizer still takes very long for the whole file at the beginning. But otherwise the undersampling seems to produce a nice stable image data and the performance seems good too. But the decorations are not really visible anymore.

@lxsk lxsk force-pushed the lxsk:show_full_minimap branch 2 times, most recently from dd8fa79 to 1ccb600 Oct 15, 2019
@lxsk

This comment has been minimized.

Copy link
Author

lxsk commented Oct 16, 2019

I only render the selection part of the decorations for very large files. The other decorations are barely noticeable anyway at those very tiny effective line heights. And these decorations were the remaining bottleneck on large files.
With this change I still don't reach the <1ms when typing, but it's <2ms for me on a lib.dom.d.ts which is 20K lines.
For files which still show all decorations (e.g. minimap.ts) the decorations add ~1ms to this render time. Slowest operation on such a file is a search for "." which takes 50-60ms to render on the initial update. Subsequent typing with such huge decorations visible render in about 6-8ms. But this is an intentional action and you get the bonus of seeing all results in the minimap, so I think it's ok to take a bit longer.
What remains relatively slow is the first renderings on a huge file, again with the 20K lines I get ~45ms for the first render and ~30ms (the whole document was updated a 2nd time). Requesting the line data is the huge part ~20ms there. All subsequent renderings, while the tokenizer still updates the document take 1-3ms.

So overall I am very happy with the results. I don't know if the highlight decorations should be handled different for large files, eg like the scroll bar.

But I can also see that files with millions of lines might still be too slow. But this option adds no real value for those files anyway. So it could fall back to the current behavior at a certain point or just disable it. I would say with such files the whole scrollbar is almost unusable and you navigate by search or something like this anyway.

@jogibear9988

This comment has been minimized.

Copy link

jogibear9988 commented Oct 16, 2019

and someone who could not live with that performance implications could still use the old behavior

fix #64459: Minimap click and drag
* new option "Minimap: Entire Document"
@lxsk lxsk force-pushed the lxsk:show_full_minimap branch from f8306b9 to ea93bbc Oct 28, 2019
@alexdima alexdima added this to the November 2019 milestone Oct 29, 2019
@alexdima alexdima modified the milestones: November 2019, January 2020 Jan 6, 2020
@jogibear9988

This comment has been minimized.

Copy link

jogibear9988 commented Feb 8, 2020

@alexdima
are there any news for this ? I think most users will not work with such large files, and still this feature could be optional.

@jrieken jrieken modified the milestones: January 2020, February 2020 Feb 10, 2020
@alexdima

This comment has been minimized.

Copy link
Member

alexdima commented Feb 17, 2020

Thank you, I have went in a slight different direction to get good performance in #90808

@alexdima alexdima closed this Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.