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

Cache obtaining patched index for twemoji #19

Merged
merged 2 commits into from Oct 17, 2022

Conversation

oprypin
Copy link
Contributor

@oprypin oprypin commented Oct 17, 2022

Otherwise this heavy operation (including a large glob and more) runs on every call to 'twemoji'

For mkdocs build of mkdocs-material-insiders, this brings the total build time from 6.7s to 4.3s

Otherwise this heavy operation (including a large glob and more) runs on every call to 'twemoji'
@gir-bot gir-bot added S: needs-review Needs to be reviewed and/or approved. C: source Related to source code. labels Oct 17, 2022
@facelessuser
Copy link
Owner

Curious, does MkDocs instantiate multiple Markdown objects or does it create one and reset it each time? If it is the first, is there a reason for that? Normally, the index is should only get built once per Markdown object. I suspect MkDocs is instantiating multiple Markdown objects though.

As far as caching here, I guess there is no problem with it, but but I would think it shouldn't be necessary, but I don't know why or how MkDocs implements everything internally.

@oprypin
Copy link
Contributor Author

oprypin commented Oct 17, 2022

Yes, MkDocs instantiates one Markdown object per page.
There is a reason for that, sure. Otherwise things like headers for ToC accumulate across pages, and who knows how many other side effects in various extensions.
Unfortunately most code assumes one Markdown object per page, it's too big of a thing to try to fight.

@oprypin
Copy link
Contributor Author

oprypin commented Oct 17, 2022

most code assumes one Markdown object per page

-that code being not in MkDocs itself; if it was just about MkDocs, I could change it quite easily

@facelessuser
Copy link
Owner

Okay, then that explains it. I never verified, but I assumed (wrongly) that the Markdown object was loaded once per site and reset between pages. Obviously, that is not the case. We'll push forward with this though. It will help alleviate build times on large projects.

@facelessuser
Copy link
Owner

Hmm, looks like we have some failures. I'll have to make sure the tip is still passing before I can make any assumptions. Assuming the tip is passing with recent versions of everything, we'll have to address the failures before we can merge.

@oprypin
Copy link
Contributor Author

oprypin commented Oct 17, 2022

For sure- I'll look into it in a day

@facelessuser
Copy link
Owner

I fixed tests on the tip. Material exposes Material, Octicons, and FontAwesome icons, but sometimes, the naming convention for some of the icons change. This will break the tests. You should be able to rebase and pick up working tests.

@oprypin
Copy link
Contributor Author

oprypin commented Oct 17, 2022

I made a merge via GitHub UI 🙂
Now CI needs to be approved again

@facelessuser
Copy link
Owner

@gir-bot lgtm

@gir-bot gir-bot added S: approved The pull request is ready to be merged. and removed S: needs-review Needs to be reviewed and/or approved. labels Oct 17, 2022
@facelessuser facelessuser merged commit 8d6c1e5 into facelessuser:master Oct 17, 2022
@oprypin oprypin deleted the opt branch October 17, 2022 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: source Related to source code. S: approved The pull request is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants