Skip to content

evaluator: Stop copying the whole heap on every formatter call - #261

Open
scottanderson wants to merge 1 commit into
WerWolv:masterfrom
scottanderson:format-heap-copy
Open

scottanderson wants to merge 1 commit into
WerWolv:masterfrom
scottanderson:format-heap-copy

Conversation

@scottanderson

Copy link
Copy Markdown
Contributor

Problem

Before calling a write formatter, a [[transform]] function or a [[format]] function, the pattern made a copy of the whole evaluator heap and put the copy back afterwards:

auto startHeap = this->m_evaluator->getHeap();
ON_SCOPE_EXIT { this->m_evaluator->getHeap() = startHeap; };

So the time to format all patterns was the number of patterns times the size of the heap. In the ImHex-Patterns test runner, mpq.hexpat on Star_103.exe runs in 25s, but then formatting its patterns takes 18.5 minutes. perf showed 98.9% of that time in memmove, called from the heap copy in callUserFormatFunc.

What the copy protects

  • Globals declared without @ each get a heap cell. The global scope is never popped, so these cells are still there when patterns are formatted after the run.
  • Function locals also get heap cells. When a function returns, the heap is cut back to its size at the start of the call, except for cells that live patterns still use.
  • A formatter can assign to a global. This writes to a heap cell that already existed before the formatter ran. The copy undid that change.

So simply cutting the heap back to its old size is not enough. That would remove new cells but let changes to globals stay. The new FormatHeapIsolation test fails with that approach.

Fix

The evaluator now has a heap guard (pushHeapGuard() / popHeapGuard()):

  • When the guard starts, it remembers the heap size.
  • The first time a cell that already existed is changed, the guard saves the cell's contents. All writes to existing cells go through getHeapCellForWriting(). These writes happen in three places: setVariable, accessData (writes, and reads that grow a cell) and getSection(HeapSectionId).
  • When the guard ends, it removes the cells created since it started and puts the saved cells back.

The result is the same as before, but the cost only depends on what the formatter changes, not on the size of the heap. With nested guards, only the innermost guard needs to save a cell, since it restores the cell before outer guards can see it.

All three places in pattern.hpp now use the guard: write formatters, [[transform]] and [[format]].

Tests

  • FormatHeapGrowth: creates a 16 MiB global string and 16384 patterns with [[format]], then formats all of them. Fails if this takes longer than 5 seconds.
  • FormatHeapIsolation: a formatter and a transform change a global u32 and a global str. A formatter that runs afterwards must still see the original values.
Before After
Formatting 16384 patterns with a 16 MiB heap 20287 ms about 52 ms
FormatHeapIsolation passes passes

All 81 PatternLanguage tests pass.

Not changed

The old copy only covered the heap, and this PR keeps it that way. Changes a formatter makes to pattern-local storage, or to a pattern's size (for example, assigning a longer string to a global str), were not undone before and still are not.

Before calling a write formatter, a [[transform]] function or a [[format]]
function, the pattern copied the whole heap and put the copy back
afterwards. The cost of formatting was the number of patterns times the size
of the heap, so a pattern with a large heap could take minutes to format.

Formatters can still change globals and other existing heap cells, so their
changes still have to be undone. Instead of copying everything, the evaluator
now keeps a heap guard. The first time a cell that already existed is
changed, the guard saves its contents. When the guard ends, it removes the
cells created since it started and puts the saved cells back. The result is
the same as before, but the cost only depends on what the formatter changes.

Formatting 16384 patterns with a 16 MiB heap now takes about 50ms instead of
about 20s.

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