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

Image.concurrency option is confusing #205

Open
ryanccn opened this issue Jan 19, 2024 · 3 comments · Fixed by #210
Open

Image.concurrency option is confusing #205

ryanccn opened this issue Jan 19, 2024 · 3 comments · Fixed by #210
Labels
enhancement New feature or request

Comments

@ryanccn
Copy link
Contributor

ryanccn commented Jan 19, 2024

The global Image.concurrency option is set as the concurrency limit for Images and not their underlying processes. An Image can have multiple formats and widths, and all of them are processed with sharp simultaneously, which can be really inefficient, increase resource usage, and reduce performance (not to mention break builds on some platforms!).

IMHO Image.concurrency should be set as the maximum concurrency of the individual output processes, not the Images. I already have a working patch for this that simply uses the global PQueue for the sharp Promises instead of the Images. Another less optimal solution could be to introduce another option that controls actual processing concurrency.

@zachleat zachleat added the enhancement New feature or request label Feb 6, 2024
@zachleat
Copy link
Member

zachleat commented Feb 6, 2024

What’s the patch look like?

@ryanccn
Copy link
Contributor Author

ryanccn commented Feb 7, 2024

Should I open a PR with this patch?

diff --git a/img.js b/img.js
index 58868a750853c658fe182e2bda56f436b38e368d..daab08a8005a56cca78b6a89735a2c2d3a72236b 100644
--- a/img.js
+++ b/img.js
@@ -578,16 +578,16 @@ class Image {
 
           if(!this.options.dryRun && stat.outputPath) {
             // Should never write when dryRun is true
-            outputFilePromises.push(sharpInstance.toFile(stat.outputPath).then(info => {
+            outputFilePromises.push(processingQueue.add(async () => sharpInstance.toFile(stat.outputPath).then(info => {
               stat.size = info.size;
               return stat;
-            }));
+            })));
           } else {
-            outputFilePromises.push(sharpInstance.toBuffer({ resolveWithObject: true }).then(({ data, info }) => {
+            outputFilePromises.push(processingQueue.add(async () => sharpInstance.toBuffer({ resolveWithObject: true }).then(({ data, info }) => {
               stat.buffer = data;
               stat.size = info.size;
               return stat;
-            }));
+            })));
           }
         }
 
@@ -689,7 +689,7 @@ function queueImage(src, opts) {
 
   debug("In-memory cache miss for %o, options: %o", src, opts);
 
-  let promise = processingQueue.add(async () => {
+  let promise = (async () => {
     if(typeof src === "string" && opts && opts.statsOnly) {
       if(Util.isRemoteUrl(src)) {
         if(!opts.remoteImageMetadata || !opts.remoteImageMetadata.width || !opts.remoteImageMetadata.height) {
@@ -713,7 +713,7 @@ function queueImage(src, opts) {
 
     let input = await img.getInput();
     return img.resize(input);
-  });
+  })();
 
   if(img.options.useCache) {
     memCache.add(key, promise);

@zachleat
Copy link
Member

Shipping with 5.0.0-alpha.1, thank you!

@zachleat zachleat removed this from the Eleventy Image v5.0.0 milestone Apr 21, 2024
@zachleat zachleat reopened this Apr 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants