Skip to content

Fix performance regression for imgmath embedding#10888

Merged
AA-Turner merged 6 commits intosphinx-doc:5.xfrom
jschueller:math_embed_v3
Oct 9, 2022
Merged

Fix performance regression for imgmath embedding#10888
AA-Turner merged 6 commits intosphinx-doc:5.xfrom
jschueller:math_embed_v3

Conversation

@jschueller
Copy link
Copy Markdown
Contributor

@jschueller jschueller commented Oct 1, 2022

We need the generated image files to be in one common folder to avoid having to re-generate several times the same image (the sha1 naming of the file acts as a cache to just read the depth)

The location should be possibly common to all workers, that's why I choosed to just keep the original destination folder, ie the file is just moved from tempdir to the build folder before.

This significantly drops my build time from 84min to 26min, but obviously leaves the unused image files in the output folder.

Another solution would be to use another global temporary folder shared across all the workers, or to add a delete pass after all workers have joined, but there does not seem to be an api to do that.

Bugfix: fix imgmath performance with embed=True
Follows #10816 #10878

@AA-Turner
Copy link
Copy Markdown
Member

This significantly drops my build time from 84min to 26min, but obviously leaves the unused image files in the output folder.

Is your project open source? 26 minutes is incredibly long, I'd like to see if there are any other optimisations that can be made here.

A

@jschueller
Copy link
Copy Markdown
Contributor Author

it's not that long considering there are almost 6k math formulas to generate
https://github.com/openturns/openturns

An idea might be generate images in batches to avoid running latex once per image

@jschueller
Copy link
Copy Markdown
Contributor Author

Also I tried compilation with mathjax, which drops compilation to 14min but validity of formulas is not checked, which is a no-go

We need the generated image files to be in one common
folder to avoid having to re-generate several times the same image
(the sha1 naming of the file acts as a cache to just read the depth)

The location should be possibly common to all workers,
that's why I choosed to just keep the original destination folder,
ie the file is just moved from tempdir to the build folder before.

This significantly drops the build times from 84min to 26min,
but obviously leaves the unused image files in the output folder.

Another solution would be to use another global temporary folder
shared across all the workers, or to add a delete pass after all workers
have joined, but there does not seem to be an api to do that.
@jschueller
Copy link
Copy Markdown
Contributor Author

jschueller commented Oct 4, 2022

Update: I added a callback to cleanup the unused latex images in the /math output folder afterwards

@jschueller jschueller changed the title imgmath: Move dest image file even with embed enabled imgmath: fix performance regression Oct 4, 2022
@jschueller jschueller changed the title imgmath: fix performance regression imgmath: fix performance regression in embed mode Oct 4, 2022
@jschueller
Copy link
Copy Markdown
Contributor Author

@AA-Turner please let me know if I should do anything else, I'd like to submit that for 5.3.0

@jschueller
Copy link
Copy Markdown
Contributor Author

cool, thanks for taking over

@AA-Turner
Copy link
Copy Markdown
Member

AA-Turner commented Oct 6, 2022

@jschueller if you could sanity check that your builds work with my changes it'd be appreciated! (Broadly I'm aiming to simplify render_math to only return a single path, making it easier to reason about. The relative path logic then happens at a higher level.)

@AA-Turner AA-Turner added this to the 5.3.0 milestone Oct 6, 2022
@jschueller
Copy link
Copy Markdown
Contributor Author

yes, I just tested it and its ok

@AA-Turner AA-Turner changed the title imgmath: fix performance regression in embed mode Fix performance regression for imgmath embedding Oct 9, 2022
@AA-Turner AA-Turner merged commit c51a88d into sphinx-doc:5.x Oct 9, 2022
@jschueller jschueller deleted the math_embed_v3 branch October 9, 2022 15:03
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Nov 9, 2022
@AA-Turner AA-Turner added extensions:mathematics The `sphinx.ext.imgmath`, `sphinx.ext.jsmath`, or `sphinx.ext.mathjax` extensions and removed extensions labels Jan 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

extensions:mathematics The `sphinx.ext.imgmath`, `sphinx.ext.jsmath`, or `sphinx.ext.mathjax` extensions type:bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants