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

typst-pdf: Build *_DEFLATED in build script instead of lazily at runtime #4522

Closed
wants to merge 2 commits into from

Conversation

bluebear94
Copy link
Contributor

No description provided.

@bluebear94 bluebear94 changed the title Build *_DEFLATED in build script instead of lazily at runtime typst-pdf: Build *_DEFLATED in build script instead of lazily at runtime Jul 8, 2024
@laurmaedje
Copy link
Member

Have you benchmarked the performance impact of this? It feels a bit like premature optimization to me.

@bluebear94
Copy link
Contributor Author

bluebear94 commented Jul 8, 2024

  • Binary size for release: 32535304 → 32530720
  • PDF export time: 17665106 ns → 17410679 ns on 9n-proto with 1 run using the --timings flag

So the performance impact is pretty small, but I don’t see any reason why the code should perform worse after this change. And getting rid of these particular Lazy statics was easy compared to some of the ones elsewhere in the codebase.

@laurmaedje
Copy link
Member

So the performance impact is pretty small, but I don’t see any reason why the code should perform worse after this change.

I'm not concerned about the code performing worse, mostly about unnecessary complexity with build.rs and slower builds because some dependencies are built for the build script (and potentially even twice if host and target mismatch, like for wasm).

@bluebear94
Copy link
Contributor Author

I'm not concerned about the code performing worse, mostly about unnecessary complexity with build.rs and slower builds because some dependencies are built for the build script (and potentially even twice if host and target mismatch, like for wasm).

Sure, that’s the one disadvantage of this change. Another option would be to perform the minifying and compression in typst-assets’s build script.

@laurmaedje
Copy link
Member

For me, personally, a build.rs needs to bring meaningful benefits to justify its existence. This does not feel like it's the case here, so I'll close this. Thanks anyway!

@laurmaedje laurmaedje closed this Jul 14, 2024
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