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.

Sign up for GitHub

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

Jump to bottom

Fix Bug #1946 #1947

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix Bug #1946 #1947

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7f48e63
Fix Bug #1946
codermapuche Jan 28, 2024
2f0b079
Update coa.js like suggeted comments
codermapuche Jan 31, 2024
a44b125
Revert changes over import
codermapuche Jan 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 25 additions & 15 deletions lib/svgo/coa.js
View file
Open in desktop
@@ -1,4 +1,5 @@
import fs from 'fs';
import os from 'os';
import path from 'path';
import colors from 'picocolors';
import { fileURLToPath } from 'url';
Expand Down Expand Up @@ -300,23 +301,32 @@ function optimizeFolder(config, dir, output) {
* @param {string} output output directory
* @return {Promise}
*/
function processDirectory(config, dir, files, output) {
async function processDirectory(config, dir, files, output) {
// take only *.svg files, recursively if necessary
var svgFilesDescriptions = getFilesDescriptions(config, dir, files, output);

return svgFilesDescriptions.length
? Promise.all(
svgFilesDescriptions.map((fileDescription) =>
optimizeFile(
config,
fileDescription.inputPath,
fileDescription.outputPath,
),
),
)
: Promise.reject(
new Error(`No SVG files have been found in '${dir}' directory.`),
);

if ( svgFilesDescriptions.length == 0 ) {
throw 'No SVG files have been found in "' + dir + '" directory.';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should still be an Error

}

let results = [ ],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Run Prettier to fix some of the failing tests, it will fix the formatting on a couple lines including this one

cpus = os.cpus().length;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth noting that since SVGO doesn't run in parallel it might not make sense to scale this by CPUs

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventloop run in single thread in fact.
The use of cpu count is a number >= 1 and <= ulimit.
The better number is ulimit, but is only available under linux platform, i dont found a cross-platform way to determine max files open limit.


while ( svgFilesDescriptions.length > 0 ) {
let _svgFilesDescriptions = svgFilesDescriptions.splice(0, cpus);

_svgFilesDescriptions = _svgFilesDescriptions.map(function(fileDescription) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this line errors or not, I would expect it to since with TS once a variable has a type you don't change it. Usually here you would create a new variable to store these results.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you are right, i dont use TS, i do this changes over the npm downloaded code

return optimizeFile(
config,
fileDescription.inputPath,
fileDescription.outputPath,
);
});

results = results.concat(await Promise.all(_svgFilesDescriptions));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slower than the existing version because each "chunk" of files takes as long as the longest file. I would suggest changing this to a worker-based method, let me know if you want me to explain

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is slower. This PR in fact run a throttle mechanism over chunks, this is the goal. Workers dont fix this, under linux the kernel has a limit over how many files can be open at same time, this limit is shared by all process of user, and another limit is shared for all process. The only way to prevent EMFILE error runing svgo over a huge number of svg is go slower and do the job in chunks.
The performance impact is minimun, runing over folder with 80k+ of svg works well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're misunderstanding what I said. Right now, it goes like "10 files open - > 6 files open - > 1 file open" if one takes longer than the rest, leading to it taking longer to start the next set of files. If you instead create 10 workers, each one will continously have 1 file open.

}

return results;
}

/**
Expand Down