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 performance regression for imgmath embedding #10888

Merged
merged 6 commits into from Oct 9, 2022

Conversation

jschueller
Copy link
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
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
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
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
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
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
Contributor Author

cool, thanks for taking over

@AA-Turner
Copy link
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
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants