Memory pressure in the GL encoder: a field report, and a question about hooks - #46
Memory pressure in the GL encoder: a field report, and a question about hooks#46cedricpinson wants to merge 5 commits into
Conversation
#programs holds promises of programs, not programs -- #loadProgram stores the async IIFE's promise so concurrent callers share one compile. dispose() passed each entry straight to gl.deleteProgram, which throws on a non-WebGLProgram, so the freeTempResources() call on the next line was unreachable. With cacheTempResources on, that means the cached render target, PBO and FBO are held for the lifetime of the GL context no matter what the caller does. There is no other way to release them: freeTempResources() is public, but a caller that follows the documented dispose() path never gets there. Two changes: free the scratch first so it no longer depends on what the program loop does, and resolve each entry before deleting it. A rejected load is swallowed -- a shader that failed to compile has nothing to delete, and dispose() is the wrong place to surface a compile error.
Two halves of one feature: the option, and the shape it actually requires. options.outputBaseLevel lets the caller's texture be LARGER than this encode. The result is written starting at that level of the caller's pyramid instead of at level 0, so a caller filling one pyramid in several passes can reuse it from the first, small pass -- without it that pass cannot take the reuse path at all and spark allocates a whole pyramid it throws away. A mismatch throws rather than quietly allocating a fresh texture: the caller named a destination. Sampler state on a caller-supplied texture is now left alone. spark has already declined to allocate its storage; overwriting its filters and wrap modes is the same trespass, and a caller driving TEXTURE_BASE_LEVEL or MIN_LOD between passes sees the reset on screen. And outputTexture is typed as SparkGLReusableOutput rather than a whole SparkGLTextureResult, because five fields are all the reuse test reads. A previous result satisfies it, which is the common case, but a caller that owns its own texture has no result object to hand back and would otherwise have to invent byteLength, srgb and a format name that nothing looks at.
The source copy is a full RGBA8 mip chain of the image being encoded -- 85 MB for a 4096 -- created and deleted on every call. cacheTempResources reaches the block render target, the PBO and the FBO, but not this, which is the largest of the four. Measured over a 21-texture load: 2136 MB of transient allocation against 102 MB, and 178 GL objects allocated against 11. Keyed on the shape, and a pooled texture is never resized. Its storage is immutable (texStorage2D + texSubImage2D), so a second texStorage2D on it is INVALID_OPERATION and a silent no-op -- the texture would keep its first shape and the encode proceed against the wrong one. That is also why the call site SKIPS texStorage2D for a pooled texture rather than letting it no-op harmlessly. Even when the shape matches and nothing breaks, the call still raises INVALID_OPERATION into the context's single shared error queue, once per pool hit, and whoever reads gl.getError() next inherits it and blames their own last call. The copy is always allocated with a full mip chain, whatever the encode's own mipmapCount is, and that is what lets the key be the shape alone: mipmapCount follows options.generateMipmaps rather than the dimensions, so the same WxH can be asked for with eleven levels or with one. Putting the count in the key would work too, but it makes the key depend on a per-call flag. The chain costs 4/3 of level 0 on encodes that did not ask for mips, and buys a key that cannot move. #fullMipCount() is now the single definition of that length, shared by the encode and the copy so they cannot drift apart. getTempResourceStats() reports what the pool did. A pool keyed on the wrong thing still shows a high hit rate while handing back the wrong shape; served next to the retained shapes is what makes that visible from outside.
Two halves of the same subject -- which levels an encode touches, and what state TEXTURE_BASE_LEVEL is left in afterwards. They are inseparable in practice: the second is only reachable because the first leaves the parameter somewhere other than 0. options.levelRange = [first, last] restricts the encode to a window, inclusive. A skipped level costs no draw, no readback and no upload. The point is not to tidy the loop but to stop re-encoding levels the caller already has: a progressive loader fills the small end of the pyramid from a small source, then the large end from the full-resolution one, and without a window the second pass re-encodes the whole chain and throws most of it away -- seven of eleven levels on a 4K BC7 texture. The source's own mip chain is still generated in full, since level `first` has to exist before it can be encoded. The result now reports levelsWritten / firstLevel / lastLevel. Under levelRange these are NOT mipmapCount levels starting at 0, and a caller reading mipmapCount alone would believe it has levels nobody encoded. The encode loop sets TEXTURE_BASE_LEVEL per level and leaves it at the last one. A freshly created texture starts at 0, so nothing ever had to reset it -- until the source copy started coming from a pool. Left at 10, the next generateMipmap sources from level 10 and derives nothing below it, so levels 1..9 keep the PREVIOUS image's content and every material after the first samples somebody else's mips. The content is not the problem and clearing it would not help: level 0 is fully overwritten and 1..N are regenerated -- but only from BASE_LEVEL up. A stale parameter with stale pixels as its symptom, seen on screen as glass and wheel rims rendering flat violet while every counter still reported 4.00x and zero fallbacks.
The cached block render target only ever grows (`cachedWidth < bw`), so a caller that encodes a small texture before a large one pays for it twice: the first encode sizes the target to fit itself, the second finds it too small, deletes it and allocates again. That is the ordinary shape of a progressive loader -- a small preview, then the full-resolution image -- and not something the caller can reorder its way out of. Deleting and recreating a multi-megabyte GPU texture mid-load is exactly the churn cacheTempResources exists to avoid. options.hintMaxTmpCacheResolution declares the largest texture the session expects to encode, in TEXELS -- the number a caller knows about its own images -- and the target is allocated at that size the first time it is needed instead of growing into it. Nothing else changes: the encode drives the viewport and the readback from the current mip's own block extents, so a target larger than the encode needs has always been legal. It is what the grow path produces one encode later anyway. A HINT, and named as one. An encode larger than it still grows the target rather than being refused, so it never behaves as a cap; what it changes is WHEN the target reaches its final size, not how big it is allowed to get. A name in terms of "max" reads as a limit being enforced, which this is not. setHintMaxTmpCacheResolution() restates it after construction, because the create option cannot always be the answer. A session that outlives the thing it encodes -- one encoder, many models -- knows the DEVICE's limits when it is built and the CONTENT's only later, and those are different numbers. It applies the next time the target is allocated or grown, which is still ahead of the first large encode, and deliberately does not reallocate an existing target: an encode may be reading it. Hint what the content needs, not what the device allows. Measured with a device cap of 8192 against assets that top out at 4096: a 2048x2048 RGBA32UI target instead of 1024x1024, 64 MB against 16 MB, +48 MB on the peak to save two allocations. With the content's own maximum instead: block render targets allocated 3 -> 1, encoder allocations 12 -> 11, peak unchanged. Only meaningful with cacheTempResources, since without it there is no cached target to size.
|
This raises some legitimate issues. The dispose code path was not tested, so it's not a surprise there were bugs. I should add some tests to validate that behavior. I've merged the fix here: #47 More control over the Reusing the source texture would be nice to bring parity against the webgpu backend, which already does that. In that case I do not maintain a pool of textures, but instead reallocate the cached texture when the layout changes. This can result in re-allocations if for example you alternate loads of 128x256 and 256x128 textures. A pool solves that, but adds complexity and can increase the total memory use. A simpler option is to track the maximum extents, so that the second allocation resizes the cache texture to 256x256. The disadvantage is that this could potentially increase memory use even more (imagine loading a 4096x1 and 1x4096 texture). I'm leaning toward the simpler solutions here, either reallocate to current texture size, or reallocate to maximum extents. Another option is to allow passing an existing GL texture as the source. This is already supported in the WebGPU side and pushes the problem of input texture management to the user. A hint for the initial allocation size is a good idea, I'll look into adding that. |
Spark's `dispose()` was always throwing trying to destroy objects that don't have a destroy method. SparkGL's `dispose()` did not wait for program promises, so it could pass a promise to `gl.deleteProgram`. Now it waits for programs to compile, so it's async, and adds a disposed flag so that attempts to compile a program after dispose immediately fail. This should address one of the main issues raised in: #46
When `cacheTempResources` is `true` SparkGL now also caches and reuses `srcTexture`. This matches the behavior of the WebGPU backend. The caching heuristic is very simple: if the new texture doesn't fit within the cached texture, then it allocates a new one. We may want to improve this in the future, for example, by ensuring the cached texture is large enough to hold all the textures seen so far, or by ensuring all the dimensions are greater or equal than a minimum cache size. Added a bunch of tests generated by Claude. This partially addresses the issues brought up in #46
Previously a sequence of encodes of increasing size (for example 64, 128, 256, 512) would reallocate at every step. To avoid this, this PR allows you to set `cacheTempResources` to an object instead of `true` to control how the cached resources are allocated. Both fields are optional and behave the same in `Spark` and `SparkGL`: - `minSize` (`number`, default: `0`) - Minimum width and height, in texels, that cached resources are allocated for. - `allocateMipmaps` (`boolean`, default: `false`) - Allocate cached resources with a full mip chain even if the encode that triggers the allocation does not generate mipmaps. Additionally, in `SparkGL` we don't call `gl.bufferData` to size the pixel buffer object on every encode anymore, but only when the buffer is created. The unintended consequence of the redundant calls was that the buffer was orphaned and a new one created. This is not the case anymore. This addresses the 5th point of #46
|
OK, the minimum allocation size is in with: #50 @cedricpinson Let me know if these changes meet your needs. I'll look into giving you more control over the |
|
I did some test with your fix and my previous pool to get some insights Tested this branch against the same harness. The bleeding is gone: all four assets are byte-identical to the uncached path, in forward order, reverse order, and largest-first. Thanks for the quick fix. One thing the numbers show, if you want it. A single slot reallocates on every change of size, and a caller encoding a model's textures meets its sizes interleaved rather than grouped, so the count follows the number of size changes rather than the number of sizes. GL objects the encoder allocates over one pass (source copies, block render targets, PBOs and FBOs; job order held fixed, so these are values rather than samples):
Single-size callers are unaffected either way. The ones that reallocate are the ones a pool helps. The cost is the one you flagged earlier in #42, that more than one source copy is retained at a time. It is bounded by the set of sizes the caller actually encodes, and in practice the extra entries are the small ones: Source copies retained at once (RGBA8, full mip chain):
I have it working locally as a I think for me it makes sense to customize the behavior for my usage, that why I feel be able to hook a function or something to customize the allocation. For most usage your fix is problably a good tradeoff. |
2 participants
Part of this experiment was done with Claude, so I dont expect you use the code as is at all, it's more an explanation about what I found.
Again thank you for spark.js it's great 🙏
What this is
Not a merge request so much as a field report with code attached. I drive SparkGL from a
progressive texture loader in a 3D viewer, and getting there needed five changes to the
encoder. A couple are plain bugs; the rest are shaped by my one use case, and I suspect the
better answer for upstream is not these patches but a few hooks. More on that at the end.
Everything below is measured. Numbers come from one asset (21 × 4096² textures, 42 encodes,
BC7) on an Apple M4 Max through ANGLE Metal, over the real viewer path. They come from a
GL-level census of every texture created, kept and written during a load, plus a render gate
that compares actual pixels against an uncompressed run. Deliberately not from the encoder's
own accounting, since the encoder is the thing under test: I've had a run report 4.00× and zero
fallbacks while drawing a visibly wrong image.
Happy to split this up, drop the parts that don't fit, or just close it and keep the discussion.
How I use SparkGL
Textures load in two passes. Pass 1 decodes a small preview (256²) so the model is on screen in
under a second; pass 2 decodes the full-resolution image. Both passes write into the same GL
texture, one immutable BC7 pyramid per texture, filled from the small end first and then from
the top, so there is never a texture swap and never a visible pop.
That shape is what most of these changes are about: the caller owns the destination pyramid and
fills it across several encodes.
For context on the payoff: 448 MB of texture memory instead of 1792 MB (4.00×), at 49.5 dB
(colour) / 48.5 dB (normal) / 44.5 dB (packed data) PSNR, for +25 to 32 ms per 4K texture.
That ratio is steady state, though, and it says nothing about the load itself. With the
intermediate render target sized in texels rather than blocks (before #43), one of these loads
allocated and freed on the order of 7 GB of scratch to produce those 448 MB. Most of what
follows is about that gap rather than about the ratio.
The changes
1.
dispose()never freed the cached scratch#programsholds promises of programs, sogl.deleteProgram(promise)throws, and it threwbefore reaching the
freeTempResources()call at the end of the function. On a session thathad encoded one 4096 texture with
cacheTempResourceson, that left the scratch render targetalive for the lifetime of the context.
Fixed by freeing the scratch first and unconditionally, and by resolving each program before
deleting it. This one is just a bug; take it in whatever form you like.
2. Let the caller own the output pyramid:
outputBaseLevel,levelRangeoutputTexturealready lets a caller hand back a previous result, but it requires the encode tomatch the texture exactly. My pass 1 encodes a 256² source into levels 4..10 of a 4096 pyramid,
so it can't take that path at all: spark allocates a full pyramid I immediately throw away.
outputBaseLevelwrites the result starting at level N of the caller's texture instead oflevel 0.
levelRange: [first, last]encodes only a window. On my pass 2 that skips seven ofeleven levels, the ones pass 1 already wrote. A skipped level costs no draw, no readback
and no upload.
levelsWritten/firstLevel/lastLevel, because under a windowthose are not
mipmapCountlevels starting at 0, and a caller readingmipmapCountalonewould think it has levels nobody encoded.
outputTextureis typed as a newSparkGLReusableOutput(5 fields) rather than a wholeSparkGLTextureResult: a caller that owns its own texture has no result object to hand back,and shouldn't have to invent
byteLength,srgband a format name the reuse test never reads.One behaviour change to flag: sampler state on a caller-supplied
outputTextureis now leftalone. That is specific to my use case. My loader drives
TEXTURE_BASE_LEVELandTEXTURE_MIN_LODon that texture between the two passes, so it needs them preserved across anencode. I doubt that generalises, and it's probably the wrong default for everyone else.
3. Pool the temporary source copy
This is the big one, and the most likely to be worth your attention regardless of what you think
of my patch.
encodeTexturecreates a full RGBA8 mip chain of the source, 85 MB for a 4096², and deletesit on every call.
cacheTempResourcesreaches the block render target, the PBO and the FBO, butnot this, which is the largest of the four.
Measured over one load of the asset above:
cacheTempResourcesonlyNote the last row. The peak does not move: the largest single encode dominates it either
way. This buys churn, not headroom, which I still think is worth having, since rapid
create/delete of multi-megabyte GPU objects is its own hazard on some drivers, and 178 to 11 is
a different kind of number from a memory saving.
The pool is keyed on the shape, retains at most one texture per shape, and
freeTempResources()drops them all. On my load: 41 of 44 source copies served from the pool (93%), 3 allocated,
one per distinct shape, which is the check that the key is right rather than merely lucky.
Two details that turned out to matter more than expected:
The source copy is always allocated with a full mip chain, even when the encode asked for
one level.
mipmapCountfollowsoptions.generateMipmapsrather than the dimensions, so thesame WxH can be asked for with eleven levels or with one, and under immutable storage a
one-level texture can't be grown to fit an eleven-level encode. Putting the level count in the
key also works (I tried it) but then the key depends on a per-call flag. The full chain costs
4/3 of level 0 (85.33 MiB against 64.00 for a 4096²) on encodes that didn't want mips, and buys
a key that can't move. This trade is the one I'm least sure about, and with
generateMipmaps: falsebeing the documented default it's a real cost for other callers, so itmay well be the wrong call for upstream.
A pooled texture must skip
texStorage2D, not let it no-op. Since #44 the source hasimmutable storage, so re-specifying it is
INVALID_OPERATIONand does nothing. Harmless to thepixels, since the pool is keyed on the shape and the storage it finds is the storage it wanted,
but it does put an error in the queue: I measured 41 spurious
INVALID_OPERATIONper load,one per pool hit. Since a WebGL context has a single shared error queue, that can be misleading
for anyone calling
gl.getError()afterwards, who has no way to tell whose error it is. Onlyrelevant here because the pool is what creates the repeats, but it seemed worth mentioning,
since a caller reading the queue can end up chasing the wrong thing.
Worth noting that #45 overlaps with this from the other end. Under
flipYthe encode currentlyreads from a second, immutable texture, and my pool deliberately leaves that one alone, since
pooling an immutable texture allocated per call is exactly the trap described above. If #45
lands, that branch disappears and the pool gets simpler, and it removes an intermediate
allocation on its own terms.
4.
TEXTURE_BASE_LEVELis left dirty by the encode loopThe loop sets
TEXTURE_BASE_LEVELper level and leaves it at the last one. A freshly createdtexture starts at 0, so nothing ever had to reset it, which is exactly why it only surfaces
once a texture is reused.
Left at 10, the next
generateMipmapsources from level 10 and derives nothing below it, solevels 1..9 keep the previous image's content. Clearing them wouldn't help: they are
regenerated, but only from
BASE_LEVELup.I reset it when handing a texture back from the pool. This is only reachable because I pool,
so it's arguably custom to my use case too, though it would be latent for any caller that reuses
a source texture, which is why it might be worth doing at the loop instead.
5. Size the scratch from the content, not by growing into it:
hintMaxTmpCacheResolutionThe cached block render target only ever grows (
cachedWidth < bw). My two passes hit the badorder every single time: pass 1 (256² to 64² blocks) sizes it, pass 2 (4096² to 1024² blocks)
finds it too small, deletes it and allocates again.
hintMaxTmpCacheResolutiondeclares the largest texture the session expects to encode, intexels, and the target is allocated at that size the first time instead of growing into it.
Nothing else changes: the encode drives the viewport and the readback from the current mip's
own block extents, so a target larger than the encode needs has always been legal. It's what the
grow path produces one encode later anyway. Result on my load: block render targets allocated
3 to 1, encoder allocations 12 to 11, peak unchanged. It avoids reallocations, that's all.
It's a hint, and named as one: an encode larger than it still grows the target rather than
being refused. Naming it
maxTmpCacheResolutionfirst was a mistake, because it read as a capbeing enforced, which it never was. What it changes is when the target reaches its final size,
not how big it may get.
setHintMaxTmpCacheResolution()restates it after construction. I need that because my encoderis created at viewer start-up, before any model is loaded: at construction the only number
available is the device's cap, and that's the wrong one to size from. With a device cap of
8192 while my assets top out at 4096, the target is allocated at 2048² RGBA32UI instead of 1024²
(64 MB against 16 MB) to save two allocations. Passing the content's own maximum instead gives
the numbers above.
Things I'm not sure about
Listing my own warts so you don't have to find them:
outputTexturethrows only whenoutputBaseLevel !== 0. With the default of 0, awrong-shaped
outputTexturestill silently allocates a fresh texture (the pre-existingbehaviour). Same caller mistake, two different outcomes, depending on an unrelated option. I
left it that way to avoid changing existing behaviour, but it's hard to defend.
levelRangeclamps silently ([0, 99]becomes[0, 10]) whileoutputBaseLevelthrows.Two neighbouring options taking opposite stances on the same class of caller error. Only
first > lastthrows.getTempResourceStats()is diagnostics, not memory reduction. I use it to check the poolfrom outside, since a pool keyed on the wrong thing still reports a healthy hit rate while
handing back the wrong shape. Easy to drop if it's not your kind of API.
generateMipmaps: falsecallers, which is the documented default.
What I actually think the answer is
I forked mainly to find out what I needed to hook in order to customise the behaviour, and
the fork is the answer to that question rather than a proposal in itself.
Now that I know, I think most of this could be exposed as a small number of extension points
rather than merged as behaviour: somewhere to plug in an allocation strategy for the temporary
resources (the pool being one implementation among others), and somewhere to control how the
compressed result lands in the destination's mip levels. Neither seems to need much added
complexity, and the defaults would do exactly what SparkGL does today, so nothing changes for
existing callers, and someone with an unusual pipeline can adapt it without forking.
That would also make the two questionable calls above go away: the full-chain source copy and
the sampler-state change are only defensible for my pipeline, and they'd belong on my side of
such a hook rather than in the library.
Branch:
cedricpinson/spark.js:reduce-memory-pressure, five commits, each with its reasoning inthe message. Happy to reshape any of it toward the hook approach if that's the direction you'd
prefer.