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

Introduce worker pools #89

Merged
merged 8 commits into from
Sep 16, 2023
Merged

Conversation

ericfennis
Copy link
Contributor

Thanks for your work on SVGFixer, great tool!
We switched to SVGFixer at lucide-icons/lucide to build our icon font.
We outline/fix more than 1200 icons to make sure our font will build correctly. Only thing we noticed is that our build times are getting out of hand. It can sometimes go up to 7 min (in Github Actions).
So I am trying to see if we can improve the build speed.

I noticed that currently, the SVGFixer is working single-threaded. And that's maybe why it takes a bit of time when outlining a lot of icons. I was thinking maybe we could switch to multi-threading to speed things up. I experimented with Piscina to create a worker pool to trace the SVG files. And the results were stunning.

Results

Tested on Macbook M2 Pro Max.

Test with Single Threaded (current SVGFixer)

Duration: 2:24.109 (m:ss.mmm)

Test with Multi Threading (current SVGFixer)

Duration: 17.158s

Screenshot:
Screenshot 2023-09-13 at 21 45 54

Curious what you think of this change!

@ericfennis
Copy link
Contributor Author

I see that builds are failing, because the worker_threads NodeJS internal module is not found.
But I also notice the CI test are testing with NodeJS version that already are End of Life.

On 11 September 2023 Node 16 went End of Life. See tweet from NodeJS. So I think we should update the workflows with latest NodeJS versions.

@Ghustavh97
Copy link
Contributor

Ghustavh97 commented Sep 14, 2023

@ericfennis This pull request is insane. Thanks for making it! Yes, please feel free to change the node versions in the action from:

node-version: [10.x, 12.x, 14.x]

to

node-version: [16.x, 18.x, 20.x]

here:

node-version: [10.x, 12.x, 14.x]

and the add engine requirements to the package.json file :

"engines" : { 
    "node" : ">=16.0.0"
 }

and then finally please create a .npmrc file in the root directory and add:

engine-strict=true

I think we should stick to 16.x for a bit longer since I know alot of projects still using it and it supports worker_threads, but if the 16.x tests fail then we will remove it.

@ericfennis
Copy link
Contributor Author

@Ghustavh97 Thanks for your comment.
Will update the workflows!

@ericfennis
Copy link
Contributor Author

@Ghustavh97 Hmm I updated the matrix in the workflows. But I think I need your help with that.
The MacOS builds are failing but I don't exactly know what the problem is.

@Ghustavh97
Copy link
Contributor

@ericfennis ok, looking into it.

@Ghustavh97 Ghustavh97 merged commit 721f865 into oslllo:master Sep 16, 2023
15 of 20 checks passed
Ghustavh97 added a commit that referenced this pull request Sep 16, 2023
@Ghustavh97
Copy link
Contributor

@ericfennis I fixed the issue in #90, thanks again for this PR, would never had the time to do it myself. You should now be able to fix your svgs faster in v3.0.0.

@ericfennis
Copy link
Contributor Author

@Ghustavh97 Awesome thanks so much!

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.

2 participants