Makes the default max_concurrent_processing half the CPUs pixa sees (runtime.GOMAXPROCS(0)), rounded down and at least 1, instead of one image per CPU. A container with no CPU limit sees every CPU of the host, so on a 48-CPU host pixa processed 48 images at once. sneak's ruling: #242 (comment).
The default is defined once, config.DefaultMaxConcurrentProcessing, read by the config and by the fallback in imageprocessor.New.
PIXA_MAX_CONCURRENT_PROCESSING and the config key stay the override. A new test holds that a value other than a whole number of at least 1 stops startup, as it already did.
"Running under upaas" gives the worst-case memory: max_concurrent_processing times 4.2 GiB, the largest peak measured over one 8192x8192 source of each source format saved at full size to every output format, plus upstream_connections times upstream_max_response_size; about 104 GiB at the defaults with 48 CPUs. It also gives the file libvips writes to /tmp for each large source, up to 1 GiB, which disappears from /tmp once written but keeps its space until the image is done.
Not visible in the diff: TestOmittedConcurrencyLimitsUseDefaults and TestNewDefaultsMaxConcurrentProcessingToCPUs (renamed ...ToHalfTheCPUs) expect the new default, as sneak allowed.
Disclosures:
Rule suppressed: paralleltest on the two new tests, which set GOMAXPROCS, which other tests read.
Deviation: comments stating the old default also changed in internal/imgcache/service.go and configs/config.example.yml.
Partly verified: memory was measured by running pixa's image processing with pixad's libvips settings and build, not through pixad's routes.
Model: opus-5-5
Makes the default `max_concurrent_processing` half the CPUs pixa sees (`runtime.GOMAXPROCS(0)`), rounded down and at least 1, instead of one image per CPU. A container with no CPU limit sees every CPU of the host, so on a 48-CPU host pixa processed 48 images at once. sneak's ruling: https://git.eeqj.de/sneak/pixa/issues/242#issuecomment-142377.
- The default is defined once, `config.DefaultMaxConcurrentProcessing`, read by the config and by the fallback in `imageprocessor.New`.
- `PIXA_MAX_CONCURRENT_PROCESSING` and the config key stay the override. A new test holds that a value other than a whole number of at least 1 stops startup, as it already did.
- "Running under upaas" gives the worst-case memory: `max_concurrent_processing` times 4.2 GiB, the largest peak measured over one 8192x8192 source of each source format saved at full size to every output format, plus `upstream_connections` times `upstream_max_response_size`; about 104 GiB at the defaults with 48 CPUs. It also gives the file libvips writes to `/tmp` for each large source, up to 1 GiB, which disappears from `/tmp` once written but keeps its space until the image is done.
Not visible in the diff: `TestOmittedConcurrencyLimitsUseDefaults` and `TestNewDefaultsMaxConcurrentProcessingToCPUs` (renamed `...ToHalfTheCPUs`) expect the new default, as sneak allowed.
Disclosures:
- Rule suppressed: `paralleltest` on the two new tests, which set `GOMAXPROCS`, which other tests read.
- Deviation: comments stating the old default also changed in `internal/imgcache/service.go` and `configs/config.example.yml`.
- Partly verified: memory was measured by running pixa's image processing with pixad's libvips settings and build, not through pixad's routes.
Model: opus-5-5
Blocked on an owner decision. Any default that stops following the CPU count fails two existing tests on a machine with more than 4 CPUs, the 48-CPU host the tests run on included, and CLAUDE.md needs your approval to change an existing test:
TestOmittedConcurrencyLimitsUseDefaults (internal/config/concurrency_limits_internal_test.go) wants an omitted max_concurrent_processing to be runtime.GOMAXPROCS(0);
TestNewDefaultsMaxConcurrentProcessingToCPUs (internal/imageprocessor/max_concurrent_processing_internal_test.go) wants New given 0 or -1 to make runtime.GOMAXPROCS(0) slots.
My reading: both checks assert the behaviour #242 removes, and the two new tests in this PR check the new default in their place. Proposed change: remove the MaxConcurrentProcessing check from the first, keeping its upstream_connections check, and drop the CPU count from its doc comment; delete the second, whose other check, that 3 makes 3 slots, TestProcessNeverExceedsMaxConcurrentProcessing also covers. With your approval the fix, the README section and the TODO.md entry follow on this branch.
Model: opus-5-5
Blocked on an owner decision. Any default that stops following the CPU count fails two existing tests on a machine with more than 4 CPUs, the 48-CPU host the tests run on included, and `CLAUDE.md` needs your approval to change an existing test:
- `TestOmittedConcurrencyLimitsUseDefaults` (`internal/config/concurrency_limits_internal_test.go`) wants an omitted `max_concurrent_processing` to be `runtime.GOMAXPROCS(0)`;
- `TestNewDefaultsMaxConcurrentProcessingToCPUs` (`internal/imageprocessor/max_concurrent_processing_internal_test.go`) wants `New` given `0` or `-1` to make `runtime.GOMAXPROCS(0)` slots.
My reading: both checks assert the behaviour https://git.eeqj.de/sneak/pixa/issues/242 removes, and the two new tests in this PR check the new default in their place. Proposed change: remove the `MaxConcurrentProcessing` check from the first, keeping its `upstream_connections` check, and drop the CPU count from its doc comment; delete the second, whose other check, that `3` makes 3 slots, `TestProcessNeverExceedsMaxConcurrentProcessing` also covers. With your approval the fix, the README section and the `TODO.md` entry follow on this branch.
Model: opus-5-5
clawbot
changed title from Hold the default number of images processed at once to 4 (closes #242) to Default max_concurrent_processing to half the CPUs pixa sees (closes #242)2026-10-15 12:06:14 +02:00
The branch is rebuilt on current next. The two cap-of-4 tests are replaced by tests that the default is half the CPUs, rounded down, and at least 1.
New test: an empty, fractional, negative or non-numeric PIXA_MAX_CONCURRENT_PROCESSING stops startup naming the variable and the key. The config loader already did this, so the test passed before the fix.
The default is imageprocessor.DefaultMaxConcurrentProcessing, used by the config and by imageprocessor.New.
The two existing tests expect the new default, as allowed; the second is renamed TestNewDefaultsMaxConcurrentProcessingToHalfTheCPUs.
README.md has the measured memory figures in "Running under upaas" and the new default in the Configuration table and the setting's entry; TODO.md has its entry.
Model: opus-5-5
Reworked after sneak's ruling (https://git.eeqj.de/sneak/pixa/issues/242#issuecomment-142377), as planned in https://git.eeqj.de/sneak/pixa/issues/242#issuecomment-142430:
- The branch is rebuilt on current `next`. The two cap-of-4 tests are replaced by tests that the default is half the CPUs, rounded down, and at least 1.
- New test: an empty, fractional, negative or non-numeric `PIXA_MAX_CONCURRENT_PROCESSING` stops startup naming the variable and the key. The config loader already did this, so the test passed before the fix.
- The default is `imageprocessor.DefaultMaxConcurrentProcessing`, used by the config and by `imageprocessor.New`.
- The two existing tests expect the new default, as allowed; the second is renamed `TestNewDefaultsMaxConcurrentProcessingToHalfTheCPUs`.
- `README.md` has the measured memory figures in "Running under upaas" and the new default in the Configuration table and the setting's entry; `TODO.md` has its entry.
Model: opus-5-5
internal/config/config.go now imports internal/imageprocessor to read DefaultMaxConcurrentProcessing. A value two packages share belongs in a third package that both import. This import also makes internal/config, and every package that imports it (internal/database, internal/middleware, internal/healthcheck), build against libvips. Acceptable: the default is defined once, in a package both import whose package doc covers it, and internal/config does not import internal/imageprocessor.
README.md, "Running under upaas", Memory: the section does not say that, while an 8192x8192 source is processed, libvips writes its decoded pixels to a temporary file in /tmp, whatever the source and output format. That is about 192 MiB for an 8-bit RGB JPEG and 512 MiB for a 16-bit RGBA PNG. The file is not in the 1.6 GiB, which counts resident memory only. It is written to the container's own filesystem, outside /var/lib/pixa and outside cache_max_bytes: up to 12 GiB at once at the defaults on a host where pixa sees 48 CPUs. Where /tmp is a tmpfs, it is memory on top of the figure. An operator sizing the container from this section is not told any of this. Acceptable: the section gives this per-image figure, says where the file is written, and says it counts as memory when /tmp is a tmpfs.
internal/imageprocessor/imageprocessor.go, doc comment of initVips: the new sentence "With one thread each, the images processed at once use no more CPUs than MaxConcurrentProcessing" is not true for GIF output. The palette step libvips runs for GIF uses one thread per CPU, and one 8192x8192 GIF conversion keeps several CPUs busy. Acceptable: the comment claims only what holds, that libvips runs one worker thread per image, or names GIF output as the exception.
Judgement call: the paralleltest suppression on the two tests that set GOMAXPROCS is needed, because tests that run in parallel read it, and it is stated; it is not a finding.
Partly verified: I checked the 1.6 GiB figure for a random-pixel 8-bit JPEG and a 16-bit RGBA PNG, each converted to every output format, not for every kind of source.
Model: opus-5-5
1. `internal/config/config.go` now imports `internal/imageprocessor` to read `DefaultMaxConcurrentProcessing`. A value two packages share belongs in a third package that both import. This import also makes `internal/config`, and every package that imports it (`internal/database`, `internal/middleware`, `internal/healthcheck`), build against libvips. Acceptable: the default is defined once, in a package both import whose package doc covers it, and `internal/config` does not import `internal/imageprocessor`.
2. `README.md`, "Running under upaas", **Memory**: the section does not say that, while an 8192x8192 source is processed, libvips writes its decoded pixels to a temporary file in `/tmp`, whatever the source and output format. That is about 192 MiB for an 8-bit RGB JPEG and 512 MiB for a 16-bit RGBA PNG. The file is not in the 1.6 GiB, which counts resident memory only. It is written to the container's own filesystem, outside `/var/lib/pixa` and outside `cache_max_bytes`: up to 12 GiB at once at the defaults on a host where pixa sees 48 CPUs. Where `/tmp` is a tmpfs, it is memory on top of the figure. An operator sizing the container from this section is not told any of this. Acceptable: the section gives this per-image figure, says where the file is written, and says it counts as memory when `/tmp` is a tmpfs.
3. `internal/imageprocessor/imageprocessor.go`, doc comment of `initVips`: the new sentence "With one thread each, the images processed at once use no more CPUs than MaxConcurrentProcessing" is not true for GIF output. The palette step libvips runs for GIF uses one thread per CPU, and one 8192x8192 GIF conversion keeps several CPUs busy. Acceptable: the comment claims only what holds, that libvips runs one worker thread per image, or names GIF output as the exception.
Judgement call: the `paralleltest` suppression on the two tests that set `GOMAXPROCS` is needed, because tests that run in parallel read it, and it is stated; it is not a finding.
Partly verified: I checked the 1.6 GiB figure for a random-pixel 8-bit JPEG and a 16-bit RGBA PNG, each converted to every output format, not for every kind of source.
Model: opus-5-5
The default is configuration, so it is defined once in internal/config (package doc: "application configuration"), next to the other defaults; imageprocessor.New's fallback for a value below 1 reads it from there. internal/config no longer imports internal/imageprocessor; it imports only internal/logger, so no package that imports it builds against libvips.
As the reviewer states it: "Running under upaas" gives the per-image temporary file figure, says it is written to /tmp in the container's own filesystem (outside /var/lib/pixa and cache_max_bytes), and says it counts as memory when /tmp is a tmpfs.
As the reviewer states it: the initVips comment claims only that libvips runs one worker thread per image, or names GIF output as the exception.
Model: opus-5-5
Decisions for the three findings:
1. The default is configuration, so it is defined once in `internal/config` (package doc: "application configuration"), next to the other defaults; `imageprocessor.New`'s fallback for a value below 1 reads it from there. `internal/config` no longer imports `internal/imageprocessor`; it imports only `internal/logger`, so no package that imports it builds against libvips.
2. As the reviewer states it: "Running under upaas" gives the per-image temporary file figure, says it is written to `/tmp` in the container's own filesystem (outside `/var/lib/pixa` and `cache_max_bytes`), and says it counts as memory when `/tmp` is a tmpfs.
3. As the reviewer states it: the `initVips` comment claims only that libvips runs one worker thread per image, or names GIF output as the exception.
Model: opus-5-5
The default is now config.DefaultMaxConcurrentProcessing, next to the other defaults in internal/config, and imageprocessor.New reads it from there. internal/config no longer imports internal/imageprocessor.
"Running under upaas" has a new Temporary files entry. It gives the file libvips writes for an 8192x8192 source, measured on pixad: 192 MiB for an 8-bit RGB JPEG, 512 MiB for a 16-bit RGBA PNG or JPEG XL, and 1 GiB for a floating-point RGBA JPEG XL, which is 24 GiB at the defaults with 48 CPUs. It says the file is in /tmp in the container's own filesystem, outside /var/lib/pixa and cache_max_bytes, and is memory where /tmp is a tmpfs.
The initVips comment now says only that libvips runs one worker thread per image.
Judgement call: I measured the /tmp sizes on the shared pixa image built earlier today, with the same libvips (8.16.1), not on an image built from this branch. This PR does not change how a source is decoded.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/pixa/pulls/261#issuecomment-142659, as decided in https://git.eeqj.de/sneak/pixa/pulls/261#issuecomment-142662:
1. The default is now `config.DefaultMaxConcurrentProcessing`, next to the other defaults in `internal/config`, and `imageprocessor.New` reads it from there. `internal/config` no longer imports `internal/imageprocessor`.
2. "Running under upaas" has a new **Temporary files** entry. It gives the file libvips writes for an 8192x8192 source, measured on pixad: 192 MiB for an 8-bit RGB JPEG, 512 MiB for a 16-bit RGBA PNG or JPEG XL, and 1 GiB for a floating-point RGBA JPEG XL, which is 24 GiB at the defaults with 48 CPUs. It says the file is in `/tmp` in the container's own filesystem, outside `/var/lib/pixa` and `cache_max_bytes`, and is memory where `/tmp` is a tmpfs.
3. The `initVips` comment now says only that libvips runs one worker thread per image.
Judgement call: I measured the `/tmp` sizes on the shared `pixa` image built earlier today, with the same libvips (8.16.1), not on an image built from this branch. This PR does not change how a source is decoded.
Model: opus-5-5
README.md, "Running under upaas", Memory and Temporary files (the same 1.6 GiB is in the TODO.md entry and the PR body): 1.6 GiB per image is not the most pixa can use. The libvips JPEG XL loader decodes the whole image into memory and then copies it to the file in /tmp, so for a JPEG XL source the decoded pixels are in memory as well as in that file. An 8192x8192 JPEG XL of 32-bit floating-point RGBA, a 13 MB file, made resident memory grow by about 2.1 GiB when saved as AVIF at full size, and by about 1.2 GiB when resized to a small WebP. An operator who sizes the container from this section runs out of memory sooner than it says. Acceptable: the per-image figure is measured over JPEG XL sources too, the floating-point RGBA one included, and Temporary files says that for a JPEG XL source the decoded pixels are held in memory as well as written to /tmp.
internal/config/config.go, DefaultMaxConcurrentProcessing: the constant cpusPerImage = 2 gives a reason nothing else in the code states, that each image uses two CPUs, while initVips says libvips runs one worker thread per image. sneak's ruling and every document say "half the CPUs". Acceptable: the code says half plainly, for example by dividing by 2 with the //nolint:mnd comment the repo already uses for such numbers, or with a name that says half.
Judgement call: a processed image stays in memory after its processing slot is freed, until it has been sent to the client (up to downstream_timeout), and the Memory formula does not count it; not a finding, because the issue's definition of done sets the formula.
Partly verified: memory was measured by running pixa's image processor directly on each file, not through pixad's routes.
Model: opus-5-5
Review failed: needs-rework.
1. `README.md`, "Running under upaas", **Memory** and **Temporary files** (the same 1.6 GiB is in the `TODO.md` entry and the PR body): 1.6 GiB per image is not the most pixa can use. The libvips JPEG XL loader decodes the whole image into memory and then copies it to the file in `/tmp`, so for a JPEG XL source the decoded pixels are in memory as well as in that file. An 8192x8192 JPEG XL of 32-bit floating-point RGBA, a 13 MB file, made resident memory grow by about 2.1 GiB when saved as AVIF at full size, and by about 1.2 GiB when resized to a small WebP. An operator who sizes the container from this section runs out of memory sooner than it says. Acceptable: the per-image figure is measured over JPEG XL sources too, the floating-point RGBA one included, and **Temporary files** says that for a JPEG XL source the decoded pixels are held in memory as well as written to `/tmp`.
2. `internal/config/config.go`, `DefaultMaxConcurrentProcessing`: the constant `cpusPerImage = 2` gives a reason nothing else in the code states, that each image uses two CPUs, while `initVips` says libvips runs one worker thread per image. sneak's ruling and every document say "half the CPUs". Acceptable: the code says half plainly, for example by dividing by 2 with the `//nolint:mnd` comment the repo already uses for such numbers, or with a name that says half.
Judgement call: a processed image stays in memory after its processing slot is freed, until it has been sent to the client (up to `downstream_timeout`), and the Memory formula does not count it; not a finding, because the issue's definition of done sets the formula.
Partly verified: memory was measured by running pixa's image processor directly on each file, not through pixad's routes.
Model: opus-5-5
As the reviewer states it, and to end the measuring: the per-image figure is the largest peak measured over one 8192x8192 source of each source format README.md lists (JPEG, PNG, GIF, WebP, AVIF, JPEG XL, with the 16-bit and 32-bit floating-point RGBA kinds where the format has them), each saved at full size to every output format. Temporary files says that for a JPEG XL source the decoded pixels are held in memory as well as written to /tmp. TODO.md and the PR body give the same figure.
As the reviewer states it: no cpusPerImage; the code says half plainly (divide by 2, with the repo's //nolint:mnd comment).
Model: opus-5-5
Decisions:
1. As the reviewer states it, and to end the measuring: the per-image figure is the largest peak measured over one 8192x8192 source of each source format `README.md` lists (JPEG, PNG, GIF, WebP, AVIF, JPEG XL, with the 16-bit and 32-bit floating-point RGBA kinds where the format has them), each saved at full size to every output format. **Temporary files** says that for a JPEG XL source the decoded pixels are held in memory as well as written to `/tmp`. `TODO.md` and the PR body give the same figure.
2. As the reviewer states it: no `cpusPerImage`; the code says half plainly (divide by 2, with the repo's `//nolint:mnd` comment).
Model: opus-5-5
The per-image figure is now 4.2 GiB, the largest peak over one 8192x8192 source of each kind the decision lists, saved at full size to every output format; it peaked for a 16-bit RGBA JPEG XL saved as WebP. README.md, TODO.md and the PR body give it, and Temporary files says a JPEG XL source's decoded pixels are held in memory as well as written to /tmp.
cpusPerImage is gone; DefaultMaxConcurrentProcessing divides by 2 with //nolint:mnd // half the CPUs.
Judgement call: Temporary files names GIF, WebP and AVIF sources besides JPEG XL, because their measurements show the decoded pixels held in memory too.
Judgement call: AVIF has no 16-bit kind. The high-bit-depth AVIF measured is 12-bit RGB: libheif refuses to decode an 8192x8192 12-bit RGBA AVIF, which needs more than its 512 MiB limit, so pixa cannot process one at all.
Partly verified: measured by running pixa's image processing directly on each file, in a container with the libvips build pixad's image uses, not through pixad's routes, which refuse an upstream on a private address.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/pixa/pulls/261#issuecomment-143153, as decided in https://git.eeqj.de/sneak/pixa/pulls/261#issuecomment-143162:
1. The per-image figure is now 4.2 GiB, the largest peak over one 8192x8192 source of each kind the decision lists, saved at full size to every output format; it peaked for a 16-bit RGBA JPEG XL saved as WebP. `README.md`, `TODO.md` and the PR body give it, and **Temporary files** says a JPEG XL source's decoded pixels are held in memory as well as written to `/tmp`.
2. `cpusPerImage` is gone; `DefaultMaxConcurrentProcessing` divides by 2 with `//nolint:mnd // half the CPUs`.
Judgement call: **Temporary files** names GIF, WebP and AVIF sources besides JPEG XL, because their measurements show the decoded pixels held in memory too.
Judgement call: AVIF has no 16-bit kind. The high-bit-depth AVIF measured is 12-bit RGB: libheif refuses to decode an 8192x8192 12-bit RGBA AVIF, which needs more than its 512 MiB limit, so pixa cannot process one at all.
Partly verified: measured by running pixa's image processing directly on each file, in a container with the libvips build pixad's image uses, not through pixad's routes, which refuse an upstream on a private address.
Model: opus-5-5
README.md, "Running under upaas", Temporary files, first sentence: libvips does not delete the file when the image is done. It removes the file's name from /tmp as soon as it has written the decoded pixels, before the image is encoded, and the file keeps its space, with no name, until the image is done. For nearly all of that time a listing of /tmp shows no file, so an operator who looks there for the up to 1 GiB per image this entry describes finds nothing. Acceptable: the entry says that the file disappears from /tmp once it is written but keeps its space, on disk or, where /tmp is a tmpfs, in memory, until the image is done.
Partly verified: I checked the 4.2 GiB figure only at a quarter of the size (4096x4096), because a run at 8192x8192 needs more memory than this review may use on the shared host.
Judgement call: the PR body is about 280 words, a little over the limit of about 250; not a finding.
Model: opus-5-5
Review failed: needs-rework.
1. `README.md`, "Running under upaas", **Temporary files**, first sentence: libvips does not delete the file when the image is done. It removes the file's name from `/tmp` as soon as it has written the decoded pixels, before the image is encoded, and the file keeps its space, with no name, until the image is done. For nearly all of that time a listing of `/tmp` shows no file, so an operator who looks there for the up to 1 GiB per image this entry describes finds nothing. Acceptable: the entry says that the file disappears from `/tmp` once it is written but keeps its space, on disk or, where `/tmp` is a tmpfs, in memory, until the image is done.
Partly verified: I checked the 4.2 GiB figure only at a quarter of the size (4096x4096), because a run at 8192x8192 needs more memory than this review may use on the shared host.
Judgement call: the PR body is about 280 words, a little over the limit of about 250; not a finding.
Model: opus-5-5
Temporary files now says the file disappears from /tmp once it is written, so a listing of /tmp does not show it, but keeps its space, on disk or, where /tmp is a tmpfs, in memory, until the image is done. No other place said when the file is deleted; the PR body now says the same and is under 250 words.
The rebase onto next put a newer TODO.md entry above this one; it is back at the top of Completed Steps.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/pixa/pulls/261#issuecomment-143704:
1. **Temporary files** now says the file disappears from `/tmp` once it is written, so a listing of `/tmp` does not show it, but keeps its space, on disk or, where `/tmp` is a tmpfs, in memory, until the image is done. No other place said when the file is deleted; the PR body now says the same and is under 250 words.
The rebase onto `next` put a newer `TODO.md` entry above this one; it is back at the top of Completed Steps.
Model: opus-5-5
An empty, fractional, negative or non-numeric value must abort startup
naming the variable and the key, never fall back to the default. The
config loader already refuses these; this pins it before the default
changes (#242).
Model: opus-5-5
An omitted max_concurrent_processing, and New given none, must be half
the CPUs Go uses, rounded down, and at least 1. The tests set GOMAXPROCS
to 1, 2, 3 and 8 and fail until the default changes
(#242).
Model: opus-5-5
The default was one image per CPU Go uses, and a container with no CPU
limit sees every CPU of the host. It is now half of runtime.GOMAXPROCS(0),
rounded down and at least 1, from imageprocessor.DefaultMaxConcurrentProcessing,
which both the config and imageprocessor.New use. The two existing tests of
the default expect the new one, as the owner allowed on the issue.
"Running under upaas" gives the worst-case memory, with a per-image peak of
about 1.6 GiB measured on pixad for an 8192x8192 source in each output
format, and tells the operator to set PIXA_MAX_CONCURRENT_PROCESSING and
PIXA_UPSTREAM_CONNECTIONS to fit the container's memory.
Model: opus-5-5
The default is configuration, so it now sits with the other defaults as
config.DefaultMaxConcurrentProcessing, and imageprocessor.New reads it
from there for a value below 1. internal/config no longer imports
internal/imageprocessor, so the packages that import internal/config do
not build against libvips.
Model: opus-5-5
The comment said the images processed at once use no more CPUs than
max_concurrent_processing, which the review found does not hold for GIF
output. It now says only that libvips runs one worker thread per image.
Model: opus-5-5
While pixa processes a large source, libvips writes its decoded pixels
to a file in /tmp, outside /var/lib/pixa and cache_max_bytes, and not
in the 1.6 GiB of resident memory. Measured on pixad for 8192x8192
sources, it is 192 MiB for an 8-bit RGB JPEG, 512 MiB for 16-bit RGBA,
and 1 GiB for a floating-point RGBA JPEG XL. The section now says so,
and that the file is memory where /tmp is a tmpfs.
Model: opus-5-5
The constant cpusPerImage gave a reason nothing else states, that each
image uses two CPUs. The function now divides by 2, with the nolint
comment the repo uses for such numbers.
Model: opus-5-5
The per-image figure is now the largest peak measured over one
8192x8192 source of each source format, with 16-bit PNG, 12-bit AVIF,
and 16-bit and 32-bit floating-point JPEG XL sources too, each saved at
full size to every output format with pixad's libvips settings: 4.2 GiB,
for the 16-bit JPEG XL saved as WebP. Temporary files says that for a
GIF, WebP, AVIF or JPEG XL source the decoded pixels are held in memory
as well as written to /tmp.
Model: opus-5-5
libvips removes the file's name from /tmp as soon as it has written the
decoded pixels, and the file keeps its space, on disk or in memory where
/tmp is a tmpfs, until the image is done. "Running under upaas" said the
file is deleted when the image is done, so an operator looking in /tmp
for it would find nothing.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Makes the default
max_concurrent_processinghalf the CPUs pixa sees (runtime.GOMAXPROCS(0)), rounded down and at least 1, instead of one image per CPU. A container with no CPU limit sees every CPU of the host, so on a 48-CPU host pixa processed 48 images at once. sneak's ruling: #242 (comment).config.DefaultMaxConcurrentProcessing, read by the config and by the fallback inimageprocessor.New.PIXA_MAX_CONCURRENT_PROCESSINGand the config key stay the override. A new test holds that a value other than a whole number of at least 1 stops startup, as it already did.max_concurrent_processingtimes 4.2 GiB, the largest peak measured over one 8192x8192 source of each source format saved at full size to every output format, plusupstream_connectionstimesupstream_max_response_size; about 104 GiB at the defaults with 48 CPUs. It also gives the file libvips writes to/tmpfor each large source, up to 1 GiB, which disappears from/tmponce written but keeps its space until the image is done.Not visible in the diff:
TestOmittedConcurrencyLimitsUseDefaultsandTestNewDefaultsMaxConcurrentProcessingToCPUs(renamed...ToHalfTheCPUs) expect the new default, as sneak allowed.Disclosures:
parallelteston the two new tests, which setGOMAXPROCS, which other tests read.internal/imgcache/service.goandconfigs/config.example.yml.Model: opus-5-5
Blocked on an owner decision. Any default that stops following the CPU count fails two existing tests on a machine with more than 4 CPUs, the 48-CPU host the tests run on included, and
CLAUDE.mdneeds your approval to change an existing test:TestOmittedConcurrencyLimitsUseDefaults(internal/config/concurrency_limits_internal_test.go) wants an omittedmax_concurrent_processingto beruntime.GOMAXPROCS(0);TestNewDefaultsMaxConcurrentProcessingToCPUs(internal/imageprocessor/max_concurrent_processing_internal_test.go) wantsNewgiven0or-1to makeruntime.GOMAXPROCS(0)slots.My reading: both checks assert the behaviour #242 removes, and the two new tests in this PR check the new default in their place. Proposed change: remove the
MaxConcurrentProcessingcheck from the first, keeping itsupstream_connectionscheck, and drop the CPU count from its doc comment; delete the second, whose other check, that3makes 3 slots,TestProcessNeverExceedsMaxConcurrentProcessingalso covers. With your approval the fix, the README section and theTODO.mdentry follow on this branch.Model: opus-5-5
6b7d939234to5c795429b80be3333730tod6602d5bf8Hold the default number of images processed at once to 4 (closes #242)to Default max_concurrent_processing to half the CPUs pixa sees (closes #242)Reworked after sneak's ruling (#242 (comment)), as planned in #242 (comment):
next. The two cap-of-4 tests are replaced by tests that the default is half the CPUs, rounded down, and at least 1.PIXA_MAX_CONCURRENT_PROCESSINGstops startup naming the variable and the key. The config loader already did this, so the test passed before the fix.imageprocessor.DefaultMaxConcurrentProcessing, used by the config and byimageprocessor.New.TestNewDefaultsMaxConcurrentProcessingToHalfTheCPUs.README.mdhas the measured memory figures in "Running under upaas" and the new default in the Configuration table and the setting's entry;TODO.mdhas its entry.Model: opus-5-5
internal/config/config.gonow importsinternal/imageprocessorto readDefaultMaxConcurrentProcessing. A value two packages share belongs in a third package that both import. This import also makesinternal/config, and every package that imports it (internal/database,internal/middleware,internal/healthcheck), build against libvips. Acceptable: the default is defined once, in a package both import whose package doc covers it, andinternal/configdoes not importinternal/imageprocessor.README.md, "Running under upaas", Memory: the section does not say that, while an 8192x8192 source is processed, libvips writes its decoded pixels to a temporary file in/tmp, whatever the source and output format. That is about 192 MiB for an 8-bit RGB JPEG and 512 MiB for a 16-bit RGBA PNG. The file is not in the 1.6 GiB, which counts resident memory only. It is written to the container's own filesystem, outside/var/lib/pixaand outsidecache_max_bytes: up to 12 GiB at once at the defaults on a host where pixa sees 48 CPUs. Where/tmpis a tmpfs, it is memory on top of the figure. An operator sizing the container from this section is not told any of this. Acceptable: the section gives this per-image figure, says where the file is written, and says it counts as memory when/tmpis a tmpfs.internal/imageprocessor/imageprocessor.go, doc comment ofinitVips: the new sentence "With one thread each, the images processed at once use no more CPUs than MaxConcurrentProcessing" is not true for GIF output. The palette step libvips runs for GIF uses one thread per CPU, and one 8192x8192 GIF conversion keeps several CPUs busy. Acceptable: the comment claims only what holds, that libvips runs one worker thread per image, or names GIF output as the exception.Judgement call: the
paralleltestsuppression on the two tests that setGOMAXPROCSis needed, because tests that run in parallel read it, and it is stated; it is not a finding.Partly verified: I checked the 1.6 GiB figure for a random-pixel 8-bit JPEG and a 16-bit RGBA PNG, each converted to every output format, not for every kind of source.
Model: opus-5-5
Decisions for the three findings:
internal/config(package doc: "application configuration"), next to the other defaults;imageprocessor.New's fallback for a value below 1 reads it from there.internal/configno longer importsinternal/imageprocessor; it imports onlyinternal/logger, so no package that imports it builds against libvips./tmpin the container's own filesystem (outside/var/lib/pixaandcache_max_bytes), and says it counts as memory when/tmpis a tmpfs.initVipscomment claims only that libvips runs one worker thread per image, or names GIF output as the exception.Model: opus-5-5
d6602d5bf8tobbc354fa99Rework for #261 (comment), as decided in #261 (comment):
config.DefaultMaxConcurrentProcessing, next to the other defaults ininternal/config, andimageprocessor.Newreads it from there.internal/configno longer importsinternal/imageprocessor./tmpin the container's own filesystem, outside/var/lib/pixaandcache_max_bytes, and is memory where/tmpis a tmpfs.initVipscomment now says only that libvips runs one worker thread per image.Judgement call: I measured the
/tmpsizes on the sharedpixaimage built earlier today, with the same libvips (8.16.1), not on an image built from this branch. This PR does not change how a source is decoded.Model: opus-5-5
Review failed: needs-rework.
README.md, "Running under upaas", Memory and Temporary files (the same 1.6 GiB is in theTODO.mdentry and the PR body): 1.6 GiB per image is not the most pixa can use. The libvips JPEG XL loader decodes the whole image into memory and then copies it to the file in/tmp, so for a JPEG XL source the decoded pixels are in memory as well as in that file. An 8192x8192 JPEG XL of 32-bit floating-point RGBA, a 13 MB file, made resident memory grow by about 2.1 GiB when saved as AVIF at full size, and by about 1.2 GiB when resized to a small WebP. An operator who sizes the container from this section runs out of memory sooner than it says. Acceptable: the per-image figure is measured over JPEG XL sources too, the floating-point RGBA one included, and Temporary files says that for a JPEG XL source the decoded pixels are held in memory as well as written to/tmp.internal/config/config.go,DefaultMaxConcurrentProcessing: the constantcpusPerImage = 2gives a reason nothing else in the code states, that each image uses two CPUs, whileinitVipssays libvips runs one worker thread per image. sneak's ruling and every document say "half the CPUs". Acceptable: the code says half plainly, for example by dividing by 2 with the//nolint:mndcomment the repo already uses for such numbers, or with a name that says half.Judgement call: a processed image stays in memory after its processing slot is freed, until it has been sent to the client (up to
downstream_timeout), and the Memory formula does not count it; not a finding, because the issue's definition of done sets the formula.Partly verified: memory was measured by running pixa's image processor directly on each file, not through pixad's routes.
Model: opus-5-5
Decisions:
README.mdlists (JPEG, PNG, GIF, WebP, AVIF, JPEG XL, with the 16-bit and 32-bit floating-point RGBA kinds where the format has them), each saved at full size to every output format. Temporary files says that for a JPEG XL source the decoded pixels are held in memory as well as written to/tmp.TODO.mdand the PR body give the same figure.cpusPerImage; the code says half plainly (divide by 2, with the repo's//nolint:mndcomment).Model: opus-5-5
bbc354fa99to296a26097d296a26097dtod9febdd87cRework for #261 (comment), as decided in #261 (comment):
README.md,TODO.mdand the PR body give it, and Temporary files says a JPEG XL source's decoded pixels are held in memory as well as written to/tmp.cpusPerImageis gone;DefaultMaxConcurrentProcessingdivides by 2 with//nolint:mnd // half the CPUs.Judgement call: Temporary files names GIF, WebP and AVIF sources besides JPEG XL, because their measurements show the decoded pixels held in memory too.
Judgement call: AVIF has no 16-bit kind. The high-bit-depth AVIF measured is 12-bit RGB: libheif refuses to decode an 8192x8192 12-bit RGBA AVIF, which needs more than its 512 MiB limit, so pixa cannot process one at all.
Partly verified: measured by running pixa's image processing directly on each file, in a container with the libvips build pixad's image uses, not through pixad's routes, which refuse an upstream on a private address.
Model: opus-5-5
Review failed: needs-rework.
README.md, "Running under upaas", Temporary files, first sentence: libvips does not delete the file when the image is done. It removes the file's name from/tmpas soon as it has written the decoded pixels, before the image is encoded, and the file keeps its space, with no name, until the image is done. For nearly all of that time a listing of/tmpshows no file, so an operator who looks there for the up to 1 GiB per image this entry describes finds nothing. Acceptable: the entry says that the file disappears from/tmponce it is written but keeps its space, on disk or, where/tmpis a tmpfs, in memory, until the image is done.Partly verified: I checked the 4.2 GiB figure only at a quarter of the size (4096x4096), because a run at 8192x8192 needs more memory than this review may use on the shared host.
Judgement call: the PR body is about 280 words, a little over the limit of about 250; not a finding.
Model: opus-5-5
d9febdd87cto0ec6cb00c0Rework for #261 (comment):
/tmponce it is written, so a listing of/tmpdoes not show it, but keeps its space, on disk or, where/tmpis a tmpfs, in memory, until the image is done. No other place said when the file is deleted; the PR body now says the same and is under 250 words.The rebase onto
nextput a newerTODO.mdentry above this one; it is back at the top of Completed Steps.Model: opus-5-5
Review passed.
Model: opus-5-5
0ec6cb00c0toa9e4d76b5fRebased onto
next; no file conflicted, and onlyTODO.mdneeded this PR's entry moved back to the top of Completed Steps.Model: opus-5-5