Skip to content

Journal: cap by serialized size and keep a recording's start through trims - #1230

Open
marcoarnulfo wants to merge 1 commit into
storytold:mainfrom
marcoarnulfo:fix/journal-size-cap
Open

marcoarnulfo wants to merge 1 commit into
storytold:mainfrom
marcoarnulfo:fix/journal-size-cap

Conversation

@marcoarnulfo

Copy link
Copy Markdown
Contributor

Part of #1228.

Thanks @ds-8362 for the measurements, they made this easy to pin down. Session::execute capped the journal only by entry count (10,000, trimmed to 5,000), so a few file.open entries with their base64 could hold tens of MB. This adds a byte cap: each entry's serialized size is measured once when it's pushed and kept alongside, and past 64 MB the oldest entries go until the journal is under half, the same way the count cap works. The newest entry always stays, even if it's bigger than the cap on its own. 64 MB is my guess at a sensible value, happy to switch to whatever you prefer.

While in there I found that trimming left Session::recording pointing at the old index, so a recording that spanned a trim saved a script with zero steps. The index now shifts with the trim (saturating at 0). Steps the trim dropped are still lost.

I left journal_params alone on purpose. automate.recordActions turns journal entries into saved scripts, and engine.journal is documented as replayable, so swapping the base64 for a summary (name, size, hash) would stop a recorded file.open or insert.picture from replaying. If you'd rather have the summary and accept that trade-off, I can do it in a follow-up. Summarising large range.setValues arrays and paginating engine.journal are also still open.

Validation:

  • tests::journal_is_capped_by_serialized_size: fails on main (12 entries of 8 MB stay in the journal, over the cap), passes here
  • tests::recording_keeps_its_steps_when_the_journal_is_trimmed: fails on main ({"recording":false,"saved":"Steps","steps":0}), passes here
  • cargo test -p gridcraft-engine --lib on the journal tests, cargo clippy -p gridcraft-engine --all-targets -- -D warnings, cargo fmt --all -- --check
  • (macOS, Apple silicon)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant