Skip to content

Fix time index reference counts - #288

Merged
mbastian merged 8 commits into
masterfrom
fix-timeindex-countmap-doubling
Aug 22, 2026
Merged

Fix time index reference counts#288
mbastian merged 8 commits into
masterfrom
fix-timeindex-countmap-doubling

Conversation

@mbastian

Copy link
Copy Markdown
Member

TimeIndexStore.countMap is a reference count per time value: remove frees the time value and recycles its slot only when the count reaches zero. Two bugs kept it from ever getting there.

Doubled on load. The time store is read before the nodes and edges, so the counts were restored from the stream and then incremented again as element insertion re-indexed the same time references. Nothing loaded from disk could be freed.

Inflated on write. ElementImpl.setTimeAttribute passed the whole map to updateIndex, so every put re-counted each time already in the map, including puts that only overwrote an existing time. removeTimeAttribute decrements one at a time, so the counts drifted upward without bound:

3 puts on a dynamic column   [3, 2, 1]
overwrite an existing key    [4, 3, 2]
canonical                    [1, 1, 1]

Changes

The index is derived state. Nothing outside the block references its slot ids, and TimeIndexStore.index/clear already maintain one count per element reference. Reading now parses the block to advance the stream and discards it, letting element insertion rebuild the index. A file written before this fix loads with canonical counts and without the time values nothing references.

AttributesImpl.setAttribute reports whether the time was new, and setTimeAttribute passes that single time to the index, matching addTime and removeTimeAttribute. Dynamic attribute columns are never value indexed, so the column index is unaffected by the narrower value.

Fixtures

The counts written to disk are now canonical, which moves one byte (12 -> 11) in each of the two 0.8 fixtures holding dynamic times. Sizes and layout are unchanged, and the field is ignored on read. The other three 0.8 fixtures and all legacy directories are byte-identical.

testTimestampStore and testIntervalStore asserted that the block round-trips its own state with no elements present. They now assert it is consumed in full and carries nothing.

Out of scope

GraphVersion double-increments on load the same way, and TextProperties width/height are written then dropped on read.

TimeIndexStore.countMap came back doubled after a load. The time store is
read before the nodes and edges, which restored the reference counts from
the stream, and inserting the elements then re-indexed their time
references and incremented the same counts again. Counts never reached
zero on remove, so time values stayed in the index once nothing
referenced them.

The index is derived state: nothing outside the block references its
slot ids, and TimeIndexStore.index/clear already maintain one count per
element reference. Reading now parses the block to advance the stream and
discards it, letting element insertion rebuild the index. Loading a store
whose counts were inflated by ElementImpl.setTimeAttribute yields
canonical counts and drops time values nothing references.

The write path is unchanged, so the byte format and the golden fixtures
stay as they are.

testTimestampStore and testIntervalStore asserted that the block
round-trips its own state with no elements present. They now assert it is
consumed in full and carries nothing.
ElementImpl.setTimeAttribute passed the whole map to updateIndex, so each
put re-counted every time already in it, including puts that only
overwrote an existing time. removeTimeAttribute decrements one at a time,
so the counts drifted upward without bound and time values stayed in the
index once nothing referenced them.

AttributesImpl.setAttribute now reports whether the time was new, and
setTimeAttribute passes that single time to the index, matching addTime
and removeTimeAttribute. Dynamic attribute columns are never value
indexed, so the column index is unaffected by the narrower value.

The counts written to disk are now canonical, which moves one byte in
each of the two 0.8 fixtures holding dynamic times. The layout is
unchanged and the field is ignored on read.
@mbastian mbastian added this to the 0.8.7 milestone Aug 22, 2026
@mbastian mbastian added the bug label Aug 22, 2026
mbastian and others added 3 commits August 22, 2026 21:07
The block is derived state: reading rebuilds it from the nodes and edges. The layout is kept, since earlier versions
read it positionally, and the fields are written empty.

Serialized bytes no longer depend on slot allocation history, so the same content always serializes to the same bytes.
The deserializers read each field through its declared type again, so a mismatched block fails at the offending field.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Assert that index content leaves no trace in the serialized bytes, and set a time the dynamic map already holds so the
round-trip count comparison exercises the write path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
getAttribute returns the instance held by the element. The types carry no reference to the store, so putting or removing
on one directly leaves the time index stale.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mbastian

Copy link
Copy Markdown
Member Author

Serialization format

The time index block is now written empty. It is derived state — reading rebuilds it from the nodes and edges — and the block layout is kept because earlier versions read it positionally.

No Serialization.VERSION bump: read compatibility is intact in both directions.

Regenerated the two time-bearing 0.8 fixtures (graph-types-timestamp 7077 → 7017 bytes, graph-types-interval 7265 → 7226). The other three carry no dynamic attributes.

Serialized bytes are now canonical. Two graphs with the same content but a different slot allocation history:

A: timestamps 1.0, 3.0                741 bytes
B: 1.0, 2.0, 3.0 then remove 2.0      741 bytes   byte-identical

These differed before, since the block carried length, garbageQueue and countMap.

Older versions reading the new format

Checked against the released jars, reading both the previous fixture and the new one, comparing countMap for the node index:

reader previous format new format
0.8.6 21 9 2 2 10 1 4 1
0.8.5 21 9 2 2 10 1 4 1
0.7.3 21 9 2 2 10 1 4 1
this branch 10 1 4 1

All three parse the new format, with the expected node count and time bounds, and land on the same counts and slot assignment as this branch ({1.0=>0, 2.0=>3, 3.0=>2, 4.0=>1}). Same result for the interval fixture. Older releases therefore get correct reference counts from a file written by this branch.

Tests

testTimestampIndexStoreContentIsNotWritten and testIntervalIndexStoreContentIsNotWritten assert that index content leaves no trace in the serialized bytes, pinning the write side by a named test rather than only by the fixture byte comparison.

newTimeIndexGraphStore now sets a time the dynamic map already holds, so testGraphStoreTimestampIndexCounts and testGraphStoreIntervalIndexCounts exercise the write path. Reverting the ElementImpl change fails them with Reference count for 5.0 expected [3] but found [1]; they stayed green before.

Docs

getAttribute returns the instance held by the element, and the time types carry no reference to the store. Documented on Element.getAttribute, TimeMap and TimeSet that putting or removing on a returned instance leaves the time index stale.

testTimestampStore and testIntervalStore already built an index with content and serialized it, so they cover the write
side too. Drops the two separate tests and the assertions that restated isEmpty.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@mbastian
mbastian merged commit 2fe6357 into master Aug 22, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant