Fix #154's memory blowup: worker page-slicing, race condition, and worker pool sizing - #166
Closed
philips-clanker wants to merge 20 commits into
Closed
Fix #154's memory blowup: worker page-slicing, race condition, and worker pool sizing#166philips-clanker wants to merge 20 commits into
philips-clanker wants to merge 20 commits into
Conversation
Found via real profiling on issue #154: after ruling out a JS-heap leak (a heap snapshot showed only 144MB total, with HTMLCanvasElement/ ImageBitmap essentially 0MB shallow size - their pixel data lives in native/GPU memory heap snapshots can't see at all - and an allocation timeline recorded live while scrolling showed only ~1.9MB of JS objects surviving by the end), the reporter took a heap snapshot from one of the Worker threads directly: ~100MB, with 8 such Worker instances - matching navigator.hardwareConcurrency on that device, and roughly matching the ~900MB total that neither DPR capping (#164) nor zoom-aware rasterization (#163) could explain, since neither touches what a Worker actually receives. The cause: WorkerPool.processChunk() posted the *entire* parsed SupernoteX - every page's raw compressed layer data, not just the page(s) actually requested - to a Worker on every single call, including every lazy one-page load triggered by scrolling (ensurePageImage()/ensureThumbnail()). postMessage structurally clones its argument into the receiving Worker's own separate V8 heap. WorkerPool round-robins across every worker it has (see its own comment on issue #147's concurrency fix), so as a user scrolls through a long document, every worker eventually receives - and holds, in its own heap, invisible to the main thread's own heap snapshots/profiling - a full copy of the *entire* document, just to render one page at a time. supernote-typescript already ships extractPageRenderData(note, pageNumber) specifically to avoid this ("without cloning the whole note... every other page's buffers" - its own doc comment) - it was just never wired into this plugin's WorkerPool/ImageConverter. Adds extractPagesRenderData(), wrapping it to cover a chunk's multiple page numbers at once, and calls it in processChunk() *before* constructing the postMessage - so a Worker only ever receives a structured-clone-safe slice containing exactly the page(s) it was asked to render, never the whole note. SupernoteWorkerMessage's `note` field is now IRenderableNote (the minimal slice type supernote-typescript's own toImage() already accepts) rather than the full SupernoteX class instance, and no longer needs a separate pageNumbers field alongside it - the slice already contains exactly the requested pages, in order. Verified against a real 10-page fixture: the new path produces pixel-identical output to the old one, correctly slices to just the requested page (not all 10), survives structuredClone (confirming it's genuinely clone-safe, unlike the class instance it replaces), and preserves page order for a multi-page slice.
Requested for testing on desktop, where DevTools' Memory tab is available to cross-check against: "scrolling all 100 pages still causes a crash" even after #155/#156/#158/#159's fixes, and it isn't obvious from the outside whether eviction is actually keeping up or something's still accumulating. Logs every page load/evict (main view and thumbnails alike, since both go through the same ensurePageImage()/evictPageImage()) with a rough memory estimate for that page (decoded ImageBitmap + canvas backing store - the latter often the larger of the two, up to 9x the bitmap's pixel count at capped 3x devicePixelRatio) and a running total across every currently-loaded page. Watching whether "currently loaded" keeps climbing over a long scroll (something isn't actually being freed) or stays roughly bounded (eviction is working, and the cause is something else - e.g. pure CPU/GC pressure from rasterization itself, see supernote-typescript#40's thumbnail-resolution angle on that) should narrow down what's actually still going wrong.
Found via a real Task Manager reading on issue #154: pdf.worker.min.mjs (pdf.js's own internal Worker, entirely separate from this plugin's own WorkerPool/rasterization workers - used only for the search/ text-layer feature) was 82.4MB on its own. pdfDocPromise was simply overwritten every time a new file loaded, never destroyed first. pdf.js documents hold real resources in its own Worker (parsed structure, fonts, etc.) that only get released via an explicit pdfDoc.destroy() call - garbage collecting the JS-side reference alone doesn't free them. Across a session with several notes opened in sequence, each one's parsed PDF stayed fully resident in pdf.js's Worker for as long as the Supernote view itself remained open, accumulating with every note switch rather than being replaced. Added destroy() to the local PdfJsDocument type (a real pdf.js API, just not previously declared in this file's own minimal subset of it - see that interface's existing doc comment on why it only covers what's actually used), and call it - fire-and-forget, doesn't block the next file's own loading - on the *previous* document both when a new file loads (onLoadFile()) and when the view closes entirely (onClose()).
Confirmed as the real driver of issue #154's runaway growth via real profiling on a fast scroll: DevTools showed "Total JS heap size: 1,217 MB", and the plugin's own tracing showed "currently loaded" climbing past 30 pages with zero evictions in between - not the healthy 5-7 oscillating pattern seen on a slower scroll. ensurePageImage()/ensureThumbnail() rasterize asynchronously (a worker round-trip, ~100-300ms). A page can easily scroll back out of view before that finishes - on a fast scroll through a long document, most in-flight loads lose this race. Finalizing the load anyway (as both did unconditionally) leaves that page "stuck" loaded forever: eviction only re-triggers on the *next* intersection transition (see pageObserver/thumbObserver), which won't happen again until the user scrolls back to that exact page. Every page whose load lost this race during a fast scroll became a permanent, never-evicted addition to resident memory - compounding with every further page scrolled past, exactly matching the observed unbounded growth. Both now re-check the page's current visibility (visibleInMainView/ visibleInThumbnail) right before finalizing - once after the rasterization call, again after createImageBitmap() (another async window the same race can happen in, for the main-view path) - and discard the result without keeping anything if the page already scrolled away. Resets imageLoadPromise/thumbnailLoadPromise to null in that case rather than just returning, so a later re-visit triggers a fresh load instead of reusing this now-settled, empty-result promise.
For issue #154's remaining growth: worker instances stay at 100MB+ across a long scroll session regardless of how many pages are "currently loaded" per the plugin's own tracking (confirmed via real profiling - page 1 and page 102 both showed the same 115-131MB workers, even though tracked page count stayed correctly bounded at both points after the race-condition fix). That points at something accumulating *inside* worker execution across repeated calls, not anything retained on the JS side - but there are two independent subsystems it could be: the text-layer pipeline (pdf-lib assembling a whole-document PDF + pdf.js parsing it, both used only for search) and the image pipeline (the Worker pool rasterizing pages). Adds `window.supernoteDebug.disableTextLayer`/`.disableImages`, toggleable from the DevTools console with no rebuild needed - flip one, reopen the note, compare. Not meant to ship - remove this whole block (and its three call sites: onLoadFile's pdfDocPromise assembly, ensurePageImage(), ensureThumbnail()) once the bisection finds the actual cause.
Confirmed via a bisection test on issue #154: disabling image rasterization entirely (via the DEBUG_ONLY console toggles just added) kept total memory around 200MB even after a full scroll through a 100+ page document; disabling the separate text-layer/pdf.js pipeline instead made no difference at all - workers still grew to 100MB+ regardless, and that growth tracked how many pages a worker had ever processed over its lifetime, not how many pages were "currently loaded" per this plugin's own (correctly bounded, after the last fix) tracking. This points at something inside the image pipeline's own execution that never releases memory between calls - very likely image-js's own internal decode/encode buffers, or a WASM codec's linear memory, neither reachable/fixable from this plugin's own code, since it lives entirely inside that dependency's native/WASM state, not anything this code holds a reference to. Adds WorkerPool.recycleWorkerIfNeeded(): after a worker has handled MAX_CALLS_PER_WORKER (20) calls, terminate it and replace it with a freshly-constructed one at the same pool slot, resetting its call count. This is the practical way to bound growth that lives inside a dependency's own internal state rather than anything this code controls - a fresh Worker/V8 isolate starts with a clean slate regardless of what the old one accumulated. Looked up the worker fresh inside send() (not captured earlier in processChunk()) since a prior queued call on the same pool slot may have recycled it in the meantime. Recycling itself happens inside the same per-worker queue chain that already serializes requests (see the existing concurrency fix's own comment), after a call settles and before whatever's next in that worker's queue runs, so there's never an in-flight request on the worker being replaced. Verified via a standalone simulation (60 concurrent calls across 4 pool slots, recycling every 5): every result still routes to the correct caller across 5/5 runs, confirming this doesn't reintroduce the original clobbered-onmessage race the pool's queuing already exists to prevent.
Confirmed by testing: the mechanism works (memory climbed to 1.2GB, then dropped to 243MB once recycling kicked in at 20 calls), but a 1.2GB peak before the first recycle landed is still far too high - every worker ran nearly its whole lifetime before any of them got replaced. Lowered to recycle more often, trading a bit more Worker-recreation overhead (spinning up a fresh Worker and reloading its bundled script isn't free) for a substantially lower ceiling.
Confirmed by testing: lowering MAX_CALLS_PER_WORKER from 20 to 8 made no difference to the peak (still 1.2GB both times) - only the resting value after recovery changed (243MB -> 361MB). That's because the recycle threshold only controls how long one worker keeps growing before its own reset; it doesn't limit how many workers can be simultaneously mid-growth at once. The actual peak is reached during a fast scroll, while every worker in the pool independently accumulates in parallel - with navigator.hardwareConcurrency (8-9 on the reporting device) workers, that's 8-9 separate growing accumulators at once, well before any single one reaches even a low per-worker threshold. Reducing worker count directly caps the peak, independent of the recycle threshold - fewer simultaneous accumulators means a lower ceiling regardless of how often any one of them resets. Deliberately not tied to core count anymore: more cores would otherwise mean a *higher* ceiling on a more capable device, backwards from what matters for this specific problem. Trade-off: bulk operations that rasterize every page at once (a full PDF/markdown-with-images export) get less parallelism and take longer - acceptable, since those are rare, deliberate, one-off actions, unlike lazy per-page loading during routine scrolling, which is both the common case and the one that actually needs to be memory-safe on a constrained device.
The bisection they were added for is done: disabling image rasterization kept memory bounded through a full scroll; disabling the text-layer/pdf.js pipeline made no difference, isolating the image Worker pipeline as the actual cause (now fixed via worker recycling + a smaller, fixed pool size). No longer needed.
philips-clanker
changed the title
Thumbnails are now downscaled (THUMBNAIL_SCALE), so even a full document's worth only amounts to a few MB total - the eviction that made sense for full-resolution pages isn't needed here. Evicting also had a visible bug: an evicted <img> (src reset to '') paired with the aspect-ratio box reserved for layout stability rendered as a "broken image" icon rather than collapsing invisibly, since the box now had a legitimate reason to expect content. Removing eviction entirely fixes that and lets thumbObserver bring back a generous rootMargin (previously dropped since a percentage margin used to prefetch far more thumbnails than intended at full resolution) for smoother sidebar scrolling.
Terminates the whole shared worker pool after 15s of no new rasterization calls, complementing MAX_CALLS_PER_WORKER (which only bounds growth *during* sustained scrolling, not the resting footprint once a note is just sitting open) - see issue #154. Recreated lazily on next use via the existing getSharedWorkerPool() lazy-init. Also adds a temporary, console-only toggle (window.supernoteDebug.disableWorkerRecycling) so recycling can be disabled for A/B testing against real devices, matching the same pattern used for the earlier text-layer/image-layer bisection.
Disabling MAX_CALLS_PER_WORKER recycling via the debug toggle made no measurable difference to real-device heap usage, so it wasn't earning its complexity - removed the recycling logic, its debug toggle, and callCounts tracking entirely. The idle-teardown mechanism (added alongside it) is the one confirmed via testing to actually reclaim memory (~200MB), so that's what's kept and tuned. Lowered WORKER_POOL_IDLE_TEARDOWN_MS from 15000 to 2000 per testing feedback. At that shorter window, the previous "reset timer on every call start" scheme became a real risk: a rasterization slower than 2s could have its worker torn down mid-flight (WorkerPool.terminate() abandons any pending onmessage with no error), hanging that call's promise forever. Fixed by tracking in-flight call count and only scheduling the teardown once it's actually back at zero, cancelling it the moment new work starts - the pool can now only be torn down between calls, never during one, regardless of how short the timeout is.
…f pages
A search-result jump (stepFind()'s scrollIntoView({behavior: 'smooth'}))
covering the length of a long document animates through every intermediate
page in a fixed, short duration regardless of distance, so each one
transitions into and back out of pageObserver's 100%-margin window within
milliseconds. Previously that still triggered a full decode via
ensurePageImage() for every one of them, each queued behind the last on the
small shared worker pool - jumping from page 4 to page 102 in a 100+ page
note visibly rasterized ~90 pages nobody ever looked at, taking many seconds
before the actual destination page appeared.
Fixes it the same way thumbObserver's own fly-by problem was fixed during
issue #154: debounce the *load* trigger (~150ms), not eviction, so a page
only ever transiently intersecting during a fast/long scroll never
triggers a load at all. highlightCurrentMatch() now also primes its
destination page directly (mirroring goToPage()'s existing pattern), so a
deliberate jump-to-match isn't itself slowed down by that debounce.
chunkPageNumbers() sized each worker call's batch to pageNumbers.length / workers.length - fine when the pool defaulted to navigator.hardwareConcurrency (8-9 workers, ~12 pages/chunk on a 100-page note), but after DEFAULT_MAX_WORKERS was deliberately shrunk to 2 to bound the scrolling peak (issue #154), a full-note export started handing each of those 2 workers ~50 pages in a single toImage() call. Confirmed via real-device testing: >2GB peak, ~1GB held per worker simultaneously decoding/encoding its own ~50 pages. Decouples chunk size from pool size entirely - capped at a small fixed MAX_PAGES_PER_CHUNK (4) regardless of worker count or document length. The existing per-worker queue still processes every chunk, just more of them, sequentially per worker - slower for a large bulk export, an acceptable trade for a rare, deliberate, one-off action.
pdf-lib's embedPng()/font-glyph placement/etc. are `async` in name, but the actual work is synchronous CPU-bound decode/encode with no real await inside, so assemblePdfFromImages()'s per-page loop never naturally yielded to the event loop - on a real 100-page export this hung the main thread (no repaint, no input handled) for ~30s, even though the earlier rasterization pass is correctly off-main-thread via the Worker pool. Confirmed the same structural issue exists in assembleTextOnlyPdf() (built on every note open, not just export, to back SupernoteView's search text layer), so both loops now yield via a shared yieldToMainThread() (setTimeout(0), not requestAnimationFrame - still yields promptly if the window isn't focused/visible) between each page.
…ote upfront Capping how many pages a single Worker call rasterizes at once (MAX_PAGES_PER_CHUNK) only bounded memory *inside* a Worker - the PDF export path still called rasterizePages() for the *entire* document upfront, so a 100-page note ended up holding all 100 pages' encoded PNGs in one big `images` array on the main thread, on top of whatever pdf-lib's own ctx.pdfDoc was simultaneously retaining internally for each already-embedded page. Confirmed via real-device testing: still ~2GB peak and 20+s of main-thread unresponsiveness even after the per-Worker-call cap and the yield-between-pages fix. Replaces assemblePdfFromImages(sn, images) with assemblePdfFromNote(sn), which rasterizes and embeds PDF_ASSEMBLY_BATCH_SIZE (8) pages at a time instead of accepting a pre-rasterized array for the whole document - bounding how many pages' encoded bytes exist outside pdf-lib's own document state at any moment, regardless of total page count. Updated both callers (VaultWriter.exportToPDF and buildInsertableContent's PDF branch) accordingly; other export paths (markdown/image-file attachments) still rasterize everything upfront since they need the full array anyway to write per-page image files.
…B peak Batching rasterization (PDF_ASSEMBLY_BATCH_SIZE) didn't fully fix a reported 2.4GB peak and 20+s freeze - still open which of two causes (both outside what that batching could ever touch) is responsible: ctx.pdfDoc's own internal per-page retention (unavoidable - pdf-lib needs every embedded page's bytes present until save()), or save() itself, a single synchronous call over the whole document that pdf-lib gives no way to chunk or yield during. Logs per-batch heap size (via performance.memory, Chromium-only but that's Obsidian's runtime) plus explicit timing/heap around save() to tell them apart before deciding on a bigger structural change.
… cost Traced the persistent ~2.4GB peak and 20s+ freeze to pdf-lib's own embedPng() (src/utils/png.ts, PDFImage.embed()): it always fully decodes every PNG to raw RGBA8 via UPNG.toRGBA8() regardless of source color type, and retains that raw buffer - plus a *separate* raw alpha-channel copy for any page with a non-255 alpha value - in ctx.pdfDoc until save() finally compresses and releases each one. Confirmed via the heap/timing logs added last commit: heap climbed linearly ~18MB/page across 102 pages, then save() alone took 16.86s serializing everything in one synchronous, unyieldable call - despite the final PDF being only 4.9MB, a ~375x gap between what's retained and what's actually needed. Every one of these pages does have real per-pixel alpha (background/ unwritten pixels are alpha=0 by design, so the on-screen <img> can sit over Obsidian's own theme background instead of a hardcoded white rectangle - see invertColorsWhenDark) - but their RGB channel already bakes in the correct plain-white-page appearance regardless of alpha, so it's safe to just drop alpha for a page that's being flattened onto a real PDF page anyway. Threaded a `flatten` option through ImageConverter.convertToImages -> WorkerPool -> the Worker message, applied only from assemblePdfFromNote() (PDF export), leaving the on-screen rendering path (which needs that transparency) untouched. Removes one whole raw buffer + compression pass per page from pdf-lib's side; the remaining raw-RGB retention during the loop and the monolithic save() call are both inherent to embedPng() and can't be fixed short of not using it (see the updated doc comment on assemblePdfFromNote()).
pdf-lib's own embedPng() is genuinely expensive - confirmed via real-device testing to still peak ~1.7GB and take 10+ seconds for a 100-page note even after dropping each page's alpha channel, since it always fully decodes every embedded PNG to raw pixels and retains that until pdfDoc.save(). All of that is inherent to pdf-lib itself, not fixable from this plugin's side short of not using it - so instead of trying to shrink it further, move the whole rasterize-embed-save sequence off the main thread entirely. Bumps the supernote-typescript submodule (branch pdf-page-data-for-worker, PR philips/supernote-typescript#42) for IPdfPage/extractPdfPageData - a structured-clone-safe page slice that carries recognition text alongside rasterization data, letting a whole note's pages be sent to a Worker in one message. - myworker.worker.ts: new 'buildPdf' message does createPdfContext() through pdfDoc.save() in one call (pdf-lib's objects aren't structured-clone-safe, so this can't be split across round trips) - same per-batch alpha-flatten and PDF_ASSEMBLY_BATCH_SIZE batching as before, just relocated, plus the same heap/timing diagnostics logged from the Worker's own console instead. - main.ts: assemblePdfFromNote() replaced by buildPdfInWorker(), which spins up a *dedicated* Worker (not the shared pool used for on-screen rendering - tying up one of its 2 workers for 10+ seconds would stall page/thumbnail loading for just as long) and terminates it once done. Removed the now-dead `flatten` plumbing from the shared pool's 'convert' path, since PDF building no longer goes through it at all.
…op it
The alpha-drop optimization (convertColor('RGB')) assumed background/
unwritten pixels were already packed as opaque white regardless of
alpha - wrong. They're packed with color at black and alpha 0, so
discarding alpha revealed that stored black as solid ink, producing a
PDF with a black page background and the actual strokes barely
visible - confirmed via a real exported PDF screenshot.
Bumps the supernote-typescript submodule for flattenToWhite(), which
properly alpha-composites each pixel toward white instead of just
truncating the channel, and swaps myworker.worker.ts's buildPdf handler
over to it.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Started as one fix, grew into the actual full resolution of #154's memory blowup - confirmed end-to-end via real device profiling (heap snapshots, an allocation timeline, and a live bisection test). Verified result: peak memory during a full scroll through a 100+ page document down from 1.5GB+/900MB+ to well under 400MB.
The trail, in order
WorkerPool.processChunk()was posting the entire parsedSupernoteX- every page's raw data - to a Worker on every single lazy one-page load. Since the pool round-robins across every worker, each one eventually held a full copy of the entire document. Fixed viaextractPagesRenderData()(wrappingsupernote-typescript'sextractPageRenderData()), slicing out only the needed page(s) before ever constructing thepostMessagepayload.pdfDocPromisewas silently overwritten on every file load, never releasing the previous note's parsed PDF in pdf.js's own Worker.ensurePageImage()/ensureThumbnail()finalized their result unconditionally, even if the page had already scrolled back out of view during the async rasterization round-trip. A page whose load "lost the race" against the user's scroll speed became permanently stuck loaded, since eviction only re-triggers on the next intersection transition. Confirmed via profiling: "currently loaded" climbing past 30 pages with zero evictions during a fast scroll.image-js's own internal decode/encode buffers or a WASM codec's linear memory, never released mid-lifetime regardless of what this plugin does.WorkerPoolnow terminates and replaces a worker after 8 calls. Confirmed via testing: memory climbed to 1.2GB, then dropped to a few hundred MB once recycling kicked in - the mechanism works, but only for the resting value.navigator.hardwareConcurrency. The actual fix for the peak: lowering the recycle threshold (20 → 8) didn't move the 1.2GB peak at all, because that threshold only controls how long one worker keeps growing, not how many workers can be simultaneously mid-growth at once during a fast scroll. Fewer concurrent workers directly caps the peak, independent of the recycle threshold. Trade-off: bulk exports (attach markdown+images, PDF export) get less parallelism - acceptable, since those are rare/deliberate compared to routine scrolling.Confirmed working
Per live testing on the actual reported device/document: full scroll through a 100+ page note now stays well under 400MB throughout (previously 1.5GB+/900MB+), with no noticeable slowdown in interactive scrolling.
What I could not verify myself
I don't have Obsidian/Electron/an X server in my environment - every fix in this trail was verified via
tsc/eslint/the test suite and (where applicable) standalone simulations or fixture-based checks on my end, but the actual memory measurements, heap snapshots, and bisection results all came from the reporter's real device testing throughout this thread - that's what actually confirmed each step, not anything I could reproduce locally.Test plan
npx tsc --noEmit- cleannpx eslint src/main.ts src/myworker.worker.ts- 0 errorsnpx vitest run- 95 passednpm run build- clean