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

Reduce max sigma for sharpen operation #3521

Closed
AmeyMore98 opened this issue Jan 8, 2023 · 9 comments
Closed

Reduce max sigma for sharpen operation #3521

AmeyMore98 opened this issue Jan 8, 2023 · 9 comments

Comments

@AmeyMore98
Copy link

AmeyMore98 commented Jan 8, 2023

Question about an existing feature

What are you trying to achieve?

I'm using sharpen with sigma: 100 on a png image(attached below). While doing this operation the RSS size keeps on increasing after which the process is OOM Killed. The OOM Kill is understandable as I've restricted the container's memory to 1.5GB. How can I fix this?

When you searched for similar issues, what did you find that might be related?

I'm running on Alpine Linux with musl libc. I found out that this could be an issue with the underlying memory allocator, hence changed the default memory allocator to mimalloc. I've also confirmed that NodeJs process is using mimalloc, with the following commands:

cat /proc/1/smaps | grep malloc:

7f3465bf1000-7f3465bf6000 r--p 00000000 ca:01 125829539                  /lib/libmimalloc.so.2.0
7f3465bf6000-7f3465c0b000 r-xp 00005000 ca:01 125829539                  /lib/libmimalloc.so.2.0
7f3465c0b000-7f3465c11000 r--p 0001a000 ca:01 125829539                  /lib/libmimalloc.so.2.0
7f3465c11000-7f3465c13000 r--p 0001f000 ca:01 125829539                  /lib/libmimalloc.so.2.0
7f3465c13000-7f3465c15000 rw-p 00021000 ca:01 125829539                  /lib/libmimalloc.so.2.0

ldd $(which node) | grep malloc :

/lib/libmimalloc.so => /lib/libmimalloc.so (0x7fb7ea34b000)

I also tried this test with jemalloc on Debian, but the same results.

I'm running on c4.8xlarge EC2 instance, with sharp concurrency as 36.
I know that lowering the concurrency could help because I tested it with concurrency 20 and it worked. However, I don't believe that is a robust solution because I assume it could then break for a larger image.

Please provide a minimal, standalone code sample, without other dependencies, that demonstrates this question

The test I'm running:

const sharp = require("sharp");
const fs = require("fs");

async function run() {
    const ipBuf = fs.readFileSync("playground-default-transformed.png");

    const opBuf = await sharp(ipBuf).sharpen({ sigma: 100 }).toBuffer();

    console.log("[DONE]");
}

function logMemory() {
    const formatMemoryUsage = (data) =>
        `${Math.round((data / 1024 / 1024) * 100) / 100} MB`;

    const memoryData = process.memoryUsage();

    const memoryUsage = {
        rss: `${formatMemoryUsage(
            memoryData.rss
        )} -> Resident Set Size - total memory allocated for the process execution`,
        heapTotal: `${formatMemoryUsage(
            memoryData.heapTotal
        )} -> total size of the allocated heap`,
        heapUsed: `${formatMemoryUsage(
            memoryData.heapUsed
        )} -> actual memory used during the execution`,
        external: `${formatMemoryUsage(
            memoryData.external
        )} -> V8 external memory`,
    };

    console.log("*".repeat(50), "\n", memoryUsage, "\n", "*".repeat(50));
}

const intr = setInterval(logMemory, 100);

run()
    .then(console.log, console.log)
    .finally(() => clearInterval(intr));

I log the memory usage at regular intervals. The tests starts with this:

{
  rss: '228.13 MB -> Resident Set Size - total memory allocated for the process execution',
  heapTotal: '8.32 MB -> total size of the allocated heap',
  heapUsed: '4.46 MB -> actual memory used during the execution',
  external: '1.43 MB -> V8 external memory'
}

...and ends here:

{
  rss: '1363.3 MB -> Resident Set Size - total memory allocated for the process execution',
  heapTotal: '8.57 MB -> total size of the allocated heap',
  heapUsed: '5 MB -> actual memory used during the execution',
  external: '1.44 MB -> V8 external memory'
}
 **************************************************
Killed

Also, here's the output of npx envinfo --binaries --system --npmPackages=sharp --npmGlobalPackages=sharp:

  System:
    OS: Linux 5.4 Alpine Linux
    CPU: (36) x64 Intel(R) Xeon(R) CPU E5-2666 v3 @ 2.90GHz
    Memory: 49.50 GB / 58.94 GB
    Container: Yes
    Shell: 1.35.0 - /bin/ash
  Binaries:
    Node: 14.21.2 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 6.14.17 - /usr/local/bin/npm
  npmPackages:
    sharp: ^0.30.7 => 0.30.7

Please provide sample image(s) that help explain this question

test-image

@lovell
Copy link
Owner

lovell commented Jan 10, 2023

Hi, thanks for reporting this. I can reproduce using vips at the command line, and it look like memory consumption for this operation (a relatively large Gaussian-based convolution) is a function of concurrency.

Downloading the test file:

$ curl -Os https://user-images.githubusercontent.com/13717006/211210991-13a20ac3-e099-4813-bf2b-894d6be414cf.png
$ file 211210991-13a20ac3-e099-4813-bf2b-894d6be414cf.png 
211210991-13a20ac3-e099-4813-bf2b-894d6be414cf.png: PNG image data, 1140 x 760, 8-bit colormap, non-interlaced

Running with default concurrency (4 in this example):

$ /usr/bin/time -f "%M" vips sharpen 211210991-13a20ac3-e099-4813-bf2b-894d6be414cf.png out.png --sigma 100
389724

Running with a forced concurrency of 36:

$ /usr/bin/time -f "%M" vips sharpen 211210991-13a20ac3-e099-4813-bf2b-894d6be414cf.png out.png --sigma 100 --vips-concurrency 36
1342504

Running with a concurrency of 4 via callgrind:

$ valgrind --tool=callgrind vips sharpen 211210991-13a20ac3-e099-4813-bf2b-894d6be414cf.png out.png --sigma 100

produces the following output:

19,781,701,012 (49.19%)  ../libvips/convolution/convi.c:vips_convi_gen'2 [/usr/local/lib/x86_64-linux-gnu/libvips.so.42.16.1]
 6,058,376,730 (15.07%)  ../libvips/conversion/extract.c:vips_extract_band_buffer [/usr/local/lib/x86_64-linux-gnu/libvips.so.42.16.1]
 5,367,557,000 (13.35%)  ../libvips/convolution/convi.c:vips_convi_gen [/usr/local/lib/x86_64-linux-gnu/libvips.so.42.16.1]
 5,047,828,724 (12.55%)  ../libvips/conversion/bandjoin.c:vips_bandjoin_buffer [/usr/local/lib/x86_64-linux-gnu/libvips.so.42.16.1]
 1,230,624,509 ( 3.06%)  ../libvips/colour/XYZ2Lab.c:vips_col_XYZ2Lab_helper [/usr/local/lib/x86_64-linux-gnu/libvips.so.42.16.1]
   443,024,820 ( 1.10%)  ../libvips/colour/LabQ2sRGB.c:vips_col_scRGB2XYZ [/usr/local/lib/x86_64-linux-gnu/libvips.so.42.16.1]

Running with a concurrency of 36 via callgrind:

$ valgrind --tool=callgrind vips sharpen 211210991-13a20ac3-e099-4813-bf2b-894d6be414cf.png out.png --sigma 100 --vips-concurrency 36

seems to deadlock, but produces the following partial output:

32,025,615,852 (48.15%)  ../libvips/convolution/convi.c:vips_convi_gen'2 [/usr/local/lib/x86_64-linux-gnu/libvips.so.42.16.1]
10,105,677,820 (15.19%)  ../libvips/conversion/extract.c:vips_extract_band_buffer [/usr/local/lib/x86_64-linux-gnu/libvips.so.42.16.1]
 8,949,432,372 (13.46%)  ../libvips/convolution/convi.c:vips_convi_gen [/usr/local/lib/x86_64-linux-gnu/libvips.so.42.16.1]
 8,419,897,704 (12.66%)  ../libvips/conversion/bandjoin.c:vips_bandjoin_buffer [/usr/local/lib/x86_64-linux-gnu/libvips.so.42.16.1]
 2,052,771,009 ( 3.09%)  ../libvips/colour/XYZ2Lab.c:vips_col_XYZ2Lab_helper [/usr/local/lib/x86_64-linux-gnu/libvips.so.42.16.1]
   738,997,560 ( 1.11%)  ../libvips/colour/LabQ2sRGB.c:vips_col_scRGB2XYZ [/usr/local/lib/x86_64-linux-gnu/libvips.so.42.16.1]

So this looks a bit like over-computation based on increased concurrency.

The behaviour of libvips 8.13 and 8.14 (and latest master) appears to be similar, so probably not related to any recent changes.

@jcupitt Do you have any insights here?

@jcupitt
Copy link
Contributor

jcupitt commented Jan 10, 2023

Wow sigma 100 on sharpen seems a lot. I think 1.5 is probably the highest value that would make sense (in my experience). It's not really designed to scale to values that high.

Could you explain why you need such a huge sigma?

@jcupitt
Copy link
Contributor

jcupitt commented Jan 10, 2023

I tried this with your test image:

$ for i in {1..100}; do echo -n "$i, "; /usr/bin/time -f %M vips sharpen sharp.png x.png --sigma $i; done
1, 184648
2, 184532
3, 186916
4, 211740
5, 207288
6, 232888
7, 251220
...
98, 1398548
99, 1467968
100, 1497140

So yes, RSS does get hilariously out of control for large values, unfortunately. It looks like it's slightly worse than linear:

x

It might be possible to improve the scaling.

@AmeyMore98
Copy link
Author

@jcupitt We allow the users to set the sigma value, and one of them set it to 100.

So what I understand is, a very high sigma along with high sharp concurrency causes this issue. So I think we will cap the value of sigma to 2, on our end. That should prevent this.

Thanks @lovell & @jcupitt !

@lovell
Copy link
Owner

lovell commented Jan 10, 2023

Thanks @jcupitt, would it make sense to reduce the accepted range for sigma in libvips? Currently 0.000001 to 10000:

https://github.com/libvips/libvips/blob/b00a4a80d6e9bcca8f2a3e02e3fcdff68e646538/libvips/convolution/sharpen.c#L333

@jcupitt
Copy link
Contributor

jcupitt commented Jan 10, 2023

Yes, maybe 0.001 to 10 would be better?

(sigma is related to display resolution -- you need about 0.5 for an 80 DPI display (typical desktop screen), or 1.5 for 300 DPI (typical high quality print))

@jcupitt
Copy link
Contributor

jcupitt commented Jan 10, 2023

@kleisauke something for your backlog :)

@lovell lovell changed the title RSS size keeps on increasing when using sharpen Reduce max sigma for sharpen operation Jan 10, 2023
@lovell lovell added this to the v0.32.0 milestone Jan 10, 2023
@lovell
Copy link
Owner

lovell commented Jan 10, 2023

Commit 081debd reduces the maximum sigma to 10 in sharp. I've left the existing minimum as-is for now.

I'll open a separate PR for libvips (for 8.15 as it'll be a breaking change).

lovell added a commit to lovell/libvips that referenced this issue Jan 10, 2023
lovell added a commit to lovell/libvips that referenced this issue Jan 23, 2023
kleisauke pushed a commit to libvips/libvips that referenced this issue Jan 25, 2023
@lovell
Copy link
Owner

lovell commented Mar 24, 2023

v0.32.0 now available with this new limit.

@lovell lovell closed this as completed Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants