perf(store): remove per-call allocations from index read hot paths - #662
perf(store): remove per-call allocations from index read hot paths#662jeswr wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Is there not already a performance test like this. Can we not update it rather than creating new ones?
| const existed = key2 in index2; | ||
| if (!existed) | ||
| index2[key2] = null; | ||
| return !existed; |
There was a problem hiding this comment.
| return !existed; | |
| return false; |
| */ | ||
| function remapEntityIds(source, target) { | ||
| const entities = source._entities, targetIds = target._ids; | ||
| const remap = Object.create(null); |
There was a problem hiding this comment.
| const remap = Object.create(null); | |
| const remap = {}; |
Why Object.create(null) here and actual objects in addToIndex?
| * already remapped when it is reached: ids are integer-index keys below | ||
| * 2^32 - 1, which `Object.keys` enumerates in ascending numeric order. | ||
| */ | ||
| function remapEntityIds(source, target) { |
There was a problem hiding this comment.
Not sure this is worth it.
- We should first investigate whether something like
WeakMapcan instead be used to enable all stores to share memory, without long term having them hold on to all references, and without a performance loss. - If an application really cares that much about performance they can follow the docs to make two stores share an index.
| * because each level must consult `remap`; sharing a generic walker with | ||
| * those hot monomorphic per-quad loops would slow them down. | ||
| */ | ||
| function crossGraphsOp(g1, g2, remap, keepIfPresent) { |
There was a problem hiding this comment.
| * injective, so the remapped quads are still unique per graph, whereas | ||
| * difference must emit the unmappable quads of the walked index. | ||
| */ | ||
| function crossGraphsIntersect(g1, g2, remap) { |
There was a problem hiding this comment.
| * builds a result store keyed by `self`'s entity index, remapping the ids of | ||
| * the walked operand into the id space of the probed one. | ||
| */ | ||
| function crossIndexOp(self, other, keepIfPresent) { |
There was a problem hiding this comment.
| // `_quadIds` maps triple terms to numeric ids through their components: | ||
| // `sId -> pId -> oId -> termId`. The object level holds the term id | ||
| // directly for a default-graph term (the only kind the parser produces) | ||
| // and spills to a graph sub-map `graphId -> termId` (with 1 for the | ||
| // default graph) once a graph-component term is interned for that triple | ||
| this._quadIds = Object.create(null); |
There was a problem hiding this comment.
This overlaps with / is made redundant by the work to have RDF 1.2 native indexing.
Two behavior-preserving changes to the N3Store read path. CPU profiles
of read-heavy downstream workloads (Comunica SPARQL over an in-memory
store, per-binding property lookups on a 38k-quad graph, CSS
parse/serialize round-trips) showed _findInIndex as the dominant read
self-time site, much of it spent re-materializing terms and collecting
the garbage that produces:
* N3EntityIndex._termFromNumericId + _termCache: memoize the
materialized term per numeric entity id. Entities are never removed
from _ids/_entities, so cached terms can never go stale. Repeated
reads reuse a single term instance instead of re-materializing per
result; memory cost is one term per distinct entity actually read.
Returned quads consequently alias term instances across reads, which
is within the RDF/JS contract: the data-model spec mandates that
"all implementations of Term (including Quad) MUST be considered
immutable, including their fields", with equivalence defined by
equals() rather than identity, and the N3.js read path already
shares instances (the DefaultGraph singleton across all reads; the
graph and outer-level terms across all quads yielded by one
_findInIndex call). The store indexes numeric ids, never term
references, so a mutated returned term can never corrupt the index
itself. A test pins that repeated reads keep returning terms equal
to freshly created ones.
* _findInIndex / _countInIndex: stop allocating a throwaway single-key
object ({ [key]: ... } + for-in) at every bound index level on every
call; iterate the bound level directly. _countInIndex is split into
monomorphic per-level helpers.
node perf/N3Store-perf.js 128 vs the merge base a2aefb9, medians of 5
interleaved runs per side (Node v25.1.0, Apple M1): fully-bound
getQuads 2.80s -> 1.19s, single-term retrievals 1.09s -> 0.83s,
two-term retrievals 1.21s -> 0.82s, 1-/2-variable finds 16-23% faster;
adds and RSS at parity. Minor-GC count over one full perf-script run
drops from 658 to 442 scavenges (--trace-gc), major GC unchanged.
The gain compounds with rdfjs#635: under its component-id triple-term
representation, term materialization becomes a recursive rebuild, which
the memoization here avoids on every repeated read.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dba0a9e to
422dd03
Compare
|
Reworked. The concrete asks are applied and the contentious pieces resolve by de-stacking:
|
|
Lets pause work on this for now and turn it back into an issue. We should land RDF 1.2 indexing first; investigate the option of 'lazy' materialisation of quads; and use a WeakMap for this kind of caching if we do pursue it to avoid memory overload issues. |
Removes per-call allocations from the
N3Storeindex read hot paths. A single commit onmain, touching onlysrc/N3Store.js; behaviour-preserving (full suite passes unchanged at 100% coverage).What it does
N3EntityIndex._termFromNumericId+_termCache: memoizes the materialized term per numeric entity id. Entities are never removed from_ids/_entities, so cached terms cannot go stale. Repeated reads reuse one term instance instead of re-materializing per result; memory cost is one term per distinct entity actually read. This is safe under the RDF/JS contract — terms are immutable (the data-model spec requires it, with equivalence byequals()not identity), and the read path already shares instances today (theDefaultGraphsingleton; the graph/outer terms within a single_findInIndexcall)._findInIndex/_countInIndex: stops allocating a throwaway single-key object ({ [key]: ... }+for-in) at every bound index level on every call, iterating the bound level directly;_countInIndexis split into monomorphic per-level helpers.Why: CPU profiles of read-heavy downstream workloads (Comunica SPARQL over an in-memory store, per-binding property lookups on a 38k-quad graph, CSS parse/serialize round-trips) showed
_findInIndexas the dominant read self-time site, much of it term re-materialization and the garbage it produces.Measured —
node perf/N3Store-perf.js 128, this branch vs its merge base, medians of 5 interleaved runs per side, Node v25.1.0, Apple M1:Minor-GC count over one full perf-script run drops from 658 to 442 scavenges (
--trace-gc); major GC unchanged.Relationship to #635: an earlier version was stacked on #635, so its diff also showed #635's commits (the cross-index set-ops, the component-id triple-term representation,
perf/N3StoreSetOps-perf.js), and its numbers were measured against that stack. It has been de-stacked and re-measured: the diff is now exactly the read-path change above, all numbers are against plainmain. The two changes are independent but compound — under #635's component-id triple-term representation term materialization becomes a recursive rebuild, which the memoization here avoids on repeated reads. The design questions raised on the old diff (WeakMap for the shared index, RDF 1.2 native-indexing overlap) concern #635's content and are best discussed there.