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

Optimize Content::has, Introspector::query_label, and MetaElem #2759

Merged
merged 13 commits into from
Nov 27, 2023

Conversation

Dherse
Copy link
Sponsor Collaborator

@Dherse Dherse commented Nov 24, 2023

See the title.

It allows has to not allocate which while a small optimization on large documents and templates should really add up.

It also optimizes Introspector::query_label to be reference based, this does several things:

  • Removes cloning for RefElem that are actually CiteElem
  • Removes one clone in RefElem when it's an actual reference
  • Removes all cloning in FootnoteElem::declaration_location

Also makes MetaElem be Unlabellable to make it smaller. I'd go as far as making it a custom impl without location, span, etc. too

@Dherse Dherse marked this pull request as ready for review November 24, 2023 21:50
@Dherse Dherse changed the title Make Content::has method use a non-allocating trait-method Optimize Content::has and Introspector::query_label Nov 25, 2023
@Dherse
Copy link
Sponsor Collaborator Author

Dherse commented Nov 25, 2023

The difference these small changes makes is actually surprising, when editing references (i.e bibliography & headings) it actually provides a surprising 11% decrease in incremental compile time, so a bigger win than one might expect, likely due to the reduced amount of cloning. It also provides a small bump in cold compile of about 8% on masterproef which also surprises me, but we take the gains we get.

@Dherse Dherse changed the title Optimize Content::has and Introspector::query_label Optimize Content::has, Introspector::query_label, and MetaElem Nov 25, 2023
@laurmaedje laurmaedje merged commit 0fbb1aa into typst:main Nov 27, 2023
3 checks passed
@laurmaedje
Copy link
Member

Thank you!

@Dherse Dherse deleted the elem-has branch November 27, 2023 11:19
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

2 participants