perf(storage): stream container listings to avoid O(children) memory - #2211
perf(storage): stream container listings to avoid O(children) memory#2211jeswr wants to merge 11 commits into
Conversation
Sorry, something went wrong.
Reading a container previously folded every child's metadata into a single N3 store and flattened it to a Quad[] before the response streamed, so peak memory was O(children): hundreds of MB to several GB of transient heap for very large containers, on every GET. Under fast in-memory locks many such reads (client listings and notification fan-out, which reads the whole container to compute an ETag) run concurrently and their peaks stack until the process is killed by the memory ceiling. DataAccessorBasedStore.getRepresentation now emits the container listing as a lazy Readable<Quad> (streamContainerRepresentation): peak memory is O(1). The full ldp:contains list lives only in the streamed body; a single ldp:contains marker for the first non-auxiliary child is kept in the response metadata so AllowAcceptHeaderWriter's empty-container check still works. Consumers that enumerated members from metadata now read them from the body: JsonResourceStorage and SingleContainerJsonStorage. Measured: a 15k-child container listing under 20 concurrent reads drops peak live heap from ~3056MB to ~140MB (flat). Full unit + integration suites pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR reduces peak memory usage for large container reads by streaming ldp:contains and child metadata quads instead of materializing full containment state in memory, and by propagating explicit “container empty” metadata for downstream HTTP header generation.
Changes:
- Stream container listing quads from DataAccessorBasedStore and make the in-memory accessor’s getChildren() lazy.
- Update JSON-based storages to consume ldp:contains statements directly from streamed quad bodies.
- Add a CI memory regression test (constrained heap) and supporting workflow/script changes.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file| File | Description |
|---|---|
| test/unit/storage/keyvalue/JsonResourceStorage.test.ts | Adds coverage ensuring container member streams are lazy and properly closed on cancellation. |
| test/unit/storage/DataAccessorBasedStore.test.ts | Adds tests for streamed container listings, cancellation cleanup, and containerEmpty metadata. |
| test/unit/storage/accessors/InMemoryDataAccessor.test.ts | Verifies child metadata is generated lazily. |
| test/unit/init/migration/SingleContainerJsonStorage.test.ts | Updates tests to read container membership from quad bodies (and ignore invalid/non-direct statements). |
| test/unit/http/output/metadata/AllowAcceptHeaderWriter.test.ts | Adds tests validating containerEmpty drives header behavior for streamed containers. |
| test/memory/container-listing.js | Adds heap-usage regression test for concurrent large container listings. |
| src/util/Vocabularies.ts | Introduces solid:containerEmpty and adds xsd:boolean to the vocabulary export. |
| src/storage/keyvalue/JsonResourceStorage.ts | Streams direct members from quad bodies instead of materializing containment metadata. |
| src/storage/DataAccessorBasedStore.ts | Streams container listing output, adds containerEmpty response metadata. |
| src/storage/accessors/InMemoryDataAccessor.ts | Switches container entries to Map and makes getChildren() lazy. |
| src/init/migration/SingleContainerJsonStorage.ts | Uses streamed containment from container quad bodies for migration listing. |
| src/http/output/metadata/AllowAcceptHeaderWriter.ts | Uses explicit containerEmpty response metadata when present. |
| package.json | Adds test:memory script with constrained heap and GC enabled. |
| .github/workflows/npm-test.yml | Runs the new memory regression test on Ubuntu + Node 20.x. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
|
@jeswr both for this and the other PR. Since you added comments to this PR afterwards, it is not clear to me if this is now ready for review or not? |
Sorry, something went wrong.
|
This PR is ready for review @joachimvh - the commits here post marking as ready-for-review were addressing the co-pilot review comments. I've marked the other as a draft, and will tag you when it is ready. |
Sorry, something went wrong.
| } | ||
|
|
||
| return representation; | ||
| private isAuxiliaryChild(child: RepresentationMetadata): boolean { |
There was a problem hiding this comment.
| private isAuxiliaryChild(child: RepresentationMetadata): boolean { | |
| private isAuxiliaryResourceMetadata(metadata: RepresentationMetadata): boolean { |
Nothing about this function is specific to child resources.
Sorry, something went wrong.
There was a problem hiding this comment.
Good point, applied in a1eb694 !
Sorry, something went wrong.
| /** | ||
| * Creates a lazy quad stream for a container listing. | ||
| */ | ||
| protected async streamContainerRepresentation(identifier: ResourceIdentifier, metadata: RepresentationMetadata): |
There was a problem hiding this comment.
This function seems overly complicated? I might be missing something, but what is the reason you can not just have a function that yields the metadata quads, and then yields all the container listing quads (with perhaps an exception for the first element because of the boolean) without all the code overhead below?
Sorry, something went wrong.
There was a problem hiding this comment.
I've cleaned this up as much as I can in 059b2ae.
The main complexity is that childQuads is initialised to determine containerEmpty; so we need to close it - this is done through the return in the finally.
Sorry, something went wrong.
|
@joachimvh - FYI, this PR is ready for re-review |
Sorry, something went wrong.
Reviewers
Requested changes must be addressed to merge this pull request.
Problem
Container reads currently collect every child's metadata in one N3 store and flatten it to a Quad[] before the response starts. Peak memory is therefore O(children), and concurrent reads can exhaust the process heap.
Change
The streamed body preserves the previous RDF output. Consumers that destroy it without reading, such as notification ETag generation, no longer enumerate the children.
Impact
The production benchmark used 20 concurrent reads of a 15,000-child container. Peak live heap dropped from roughly 3,056 MB to 140 MB. The CI workload runs the same shape with a 128 MB old-space limit; local runs peak at 23–36 MB, while the previous implementation runs out of memory.
Tests