-
Notifications
You must be signed in to change notification settings - Fork 3
new argument: --batches <batches>: process splits in parallel instead of linearly #21
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
base: master
Are you sure you want to change the base?
Conversation
|
Summary of ChangesHello @helpimnotdrowning, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a major performance enhancement by parallelizing the font splitting process. It adds a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces parallel processing for font subset generation, which is a great performance improvement. The implementation uses p-limit to control concurrency, which is appropriate for this task. I've made a few suggestions to improve code clarity and maintainability, such as removing unused parameters and commented-out code. I also have a suggestion to improve the progress logging to make it more accurate in a parallel execution context. Overall, this is a solid contribution.
|
the swap-around of the logging order mentioned above was an opinionated change based on personal belief that logs should state what the program is about to do rather than what it has just done (I find it helps in debugging and tracking down issue sources). I made this swap before I thought to publish my changes at all; if you (maintainer) would like me to put it back to before I can do that, but I think it's for the best to stay like it is anyways. (the AI explanation is also wrong, it says that "several tasks might be logged as 'started' while they are actually waiting in a queue": |
New argument
--batches <batches: int || 4>will parallelize the splits usingp-limit(3.1, compatible with node 10). This runs much faster on my machine: using the font "IBM Plex Sans JP" complete/hinted at-c 128, current takes about 1minute/split for 157 splits (took too long for me to keep measuring, est. 2h40m); using the default batch size of 4 brings it up to around 3minutes/split, taking only 44 minutes.