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

feat: parallel rendering of latex snippets #1372

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jonboh
Copy link

@jonboh jonboh commented Mar 31, 2024

Hi! I've been using the latex renderer for some of my notes and I noticed its performance scaled badly with the amount of equations in the document. In short, equations currently are rendered one by one, in a blocking manner.
This PR performs all the renderings in parallel.

Here is an example of the difference:
Current main behavior (around 13s until the user gets back control):
https://github.com/nvim-neorg/neorg/assets/31407988/f004090c-92bb-4f67-8187-d31997b05b27
This pr (3 to 4s until the user gets back control):
https://github.com/nvim-neorg/neorg/assets/31407988/1104073e-700b-44c1-a5b1-e41abf2592c3

I've made a bunch of changes to the latex renderer that I think improve its usability:

  1. Refactor latex_renderer so that all latex_snippets are extracted and rendered in parallel jobs. This way we only need to wait for the longest render.
  2. Name the rendered files according to the base64 encoding of the snippet, this way subsequent calls to render_latex do not need to perform the conversion from latex to png. I've added the same base64 utility function that is used in image.nvim. Let me know if it would be better placed in another location in the project or if it is better to use other implementation (I'm not very familiar with the lua ecosystem).

EDIT:
Point 3 is no longer part of this PR, as @benlubas correctly pointed out that the performance problems that I was experiencing with OnCursorMove only apply when you don't set the conceal option, I keep the text here for context:
3. Remove the "OnCursorMove" event handling. Unfortunately it is too costly whenever you have more than 5 or 6 equations on screen. If you try to open a document with ~10 equations like in the example you can end up with the nvim ui effectively locked if you try to move your cursor too quickly. In addition this event does not prevent the user from having to re-run latex_render if they are making modifications to the equations, as new equations won't get rendered on OnCursorMove event. I think it is better to just make the user re-run latex_render, but I'm open to revert this change if you consider it's still better to have the event as it is on main.

@jonboh jonboh changed the title feat: latex parallel rendering feat: parallel rendering of latex snippets Mar 31, 2024
@benlubas
Copy link
Contributor

@jonboh could you upload the large latex file? I've been working on improving this module as well and I'd like to test with a large file b/c currently my implementation doesn't hang neovim at all.

@jonboh
Copy link
Author

jonboh commented Mar 31, 2024

sure, here's the file I've been using for testing:

* Test
  $\min_\theta \mathbb{E} -\log p_\theta(x)$
  $\min_\theta \mathbb{E} -\log p_\theta(x)$
  $e^{i\pi} + 1 = 0$
  $a^2 + b^2 = c^2$
  $E = mc^2$
  $F = ma$
  $\hat{H}\psi = E\psi$
  $\nabla \cdot \mathbf{E} = \frac{\rho}{\varepsilon_0}$
  $\nabla \cdot \mathbf{B} = 0$
  $dS \geq \frac{dQ}{T}$
  $\gamma = \frac{1}{\sqrt{1 - \frac{v^2}{c^2}}}$
  $(i\gamma^\mu\partial_\mu - m)\psi = 0$

@jonboh
Copy link
Author

jonboh commented Mar 31, 2024

@benlubas have you uploaded your changes to your fork? I'll be happy to give them a try If you don't mind, there are still some things in this pr that don't work great, wheneve the images are moved by a line there's a slight delay, which is noticeable if you keep pressing Down, but I think that is related to image.nvim.

@benlubas
Copy link
Contributor

I'm not sure if you're testing on nightly with the inline conceal, but when I use this branch, this happens after rendering and then making a change to the text:
image

@benlubas
Copy link
Contributor

I'm in the middle of rewriting mine to fix the move up/down flickering issue.

I can go grab a revision that's working and link it here for you to try though! It takes advantage of a change that was just merged into image.nvim yesterday, so make sure you update image.nvim.

https://github.com/benlubas/neorg/tree/feat/async_images

Here, this should work actually I haven't pushed anything breaking. I will warn you that with a large number of images it starts to get really really slow, it's definitely still a WIP.

The things that are working: inline images with conceal. conceal/unconceal on cursor move. updates the images when you change text. Images are displayed accurately even if there are virtual lines above the image.

@jonboh
Copy link
Author

jonboh commented Mar 31, 2024

I'm trying the conceal again and surprisingly a lot of the performance problems that appear with several equations are not present this way...
I'm going to re-add the OnCursorMove, as it seems to work well, and in that case the clearing after text edits works ok.
The sequential processing of the latex still offers quite a speed-up.
I'll try to check your branch tomorrow and see if we have common things that could work together.

@benlubas
Copy link
Contributor

My branch is going to change a lot in a few days, I might work on it a bunch more today now though

@max397574
Copy link
Contributor

I think we shouldn't write a base64 module here
We should rather use vim.base64 from this pr neovim/neovim#25843 which likely is just as fast since it's written in C

@jonboh
Copy link
Author

jonboh commented Mar 31, 2024

I wasn't aware of vim.base64, thanks, that makes things easier. I've removed the base64 file.

@jonboh
Copy link
Author

jonboh commented Apr 1, 2024

I've added as well a fix to handle cases in which the math snippet does not compile to an image, for example $\e$. In main this case throws an "attemp to index a nil value".

Now a small log message will be generated indicating which snippet could no be compiled.

@benlubas
Copy link
Contributor

benlubas commented Apr 4, 2024

@jonboh Just opened a PR for image.nvim that can fix a lot of the performance issues I was having. It should also fix your performance issues. 3rd/image.nvim#141

try it out, and let me know how it goes. (it might break something, but I couldn't think of anything that would break)

@benlubas benlubas mentioned this pull request Apr 19, 2024
5 tasks
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.

None yet

3 participants