From c23bb693b89d4b321a5e536d654f0812e66f8628 Mon Sep 17 00:00:00 2001 From: Scott Anderson <662325+scottanderson@users.noreply.github.com> Date: Tue, 29 Sep 2026 00:15:55 -0400 Subject: [PATCH] evaluator: Stop copying the whole heap on every formatter call 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. --- lib/include/pl/core/evaluator.hpp | 31 ++++ lib/include/pl/patterns/pattern.hpp | 12 +- lib/source/pl/core/evaluator.cpp | 35 ++++- tests/CMakeLists.txt | 3 + .../test_pattern_format_heap.hpp | 144 ++++++++++++++++++ tests/source/test_pattern_format_heap.cpp | 24 +++ tests/source/tests.cpp | 3 + 7 files changed, 242 insertions(+), 10 deletions(-) create mode 100644 tests/include/test_patterns/test_pattern_format_heap.hpp create mode 100644 tests/source/test_pattern_format_heap.cpp diff --git a/lib/include/pl/core/evaluator.hpp b/lib/include/pl/core/evaluator.hpp index b55c1a1b..3d4c075c 100644 --- a/lib/include/pl/core/evaluator.hpp +++ b/lib/include/pl/core/evaluator.hpp @@ -393,6 +393,28 @@ namespace pl::core { return this->m_heap; } + /** + * @brief Starts recording heap changes so popHeapGuard() can undo them. + * Only cells that get written are copied, so the cost does not grow with + * the size of the heap. + */ + void pushHeapGuard(); + + /** + * @brief Undoes all heap changes made since the matching pushHeapGuard(). + * New cells are removed and changed cells get their old contents back. + */ + void popHeapGuard(); + + /** + * @brief Gets a heap cell that is about to be changed. If a heap guard is + * active, the cell's current contents are saved first so the guard can + * restore them. + * @param address Address of the heap cell + * @return The heap cell + */ + [[nodiscard]] std::vector &getHeapCellForWriting(u64 address); + [[nodiscard]] std::map &getPatternLocalStorage() { return this->m_patternLocalStorage; } @@ -606,6 +628,15 @@ namespace pl::core { std::map m_heapReferenceCounts; std::map m_patternLocalStorage; + /** + * @brief Heap state saved by pushHeapGuard() and restored by popHeapGuard() + */ + struct HeapGuard { + size_t heapSize; + std::map> originalCells; + }; + std::vector m_heapGuards; + std::map> m_attributedPatterns; std::vector> m_scopes; std::vector> m_patterns; diff --git a/lib/include/pl/patterns/pattern.hpp b/lib/include/pl/patterns/pattern.hpp index e2574e05..fcf5c899 100644 --- a/lib/include/pl/patterns/pattern.hpp +++ b/lib/include/pl/patterns/pattern.hpp @@ -468,8 +468,8 @@ namespace pl::ptrn { try { const auto function = this->m_evaluator->findFunction(formatterFunctionName); if (function.has_value()) { - auto startHeap = this->m_evaluator->getHeap(); - ON_SCOPE_EXIT { this->m_evaluator->getHeap() = startHeap; }; + this->m_evaluator->pushHeapGuard(); + ON_SCOPE_EXIT { this->m_evaluator->popHeapGuard(); }; auto formatterResult = function->func(this->m_evaluator, { value }); if (formatterResult.has_value()) { @@ -590,8 +590,8 @@ namespace pl::ptrn { auto evaluator = this->getEvaluator(); if (auto transformFunc = evaluator->findFunction(this->getTransformFunction()); transformFunc.has_value()) { - auto startHeap = this->m_evaluator->getHeap(); - ON_SCOPE_EXIT { this->m_evaluator->getHeap() = startHeap; }; + this->m_evaluator->pushHeapGuard(); + ON_SCOPE_EXIT { this->m_evaluator->popHeapGuard(); }; // Preserve pattern variable name std::string patternName; @@ -621,8 +621,8 @@ namespace pl::ptrn { else { const auto function = this->m_evaluator->findFunction(formatterFunctionName); if (function.has_value()) { - auto startHeap = this->m_evaluator->getHeap(); - ON_SCOPE_EXIT { this->m_evaluator->getHeap() = startHeap; }; + this->m_evaluator->pushHeapGuard(); + ON_SCOPE_EXIT { this->m_evaluator->popHeapGuard(); }; // Preserve pattern variable name std::string patternName; diff --git a/lib/source/pl/core/evaluator.cpp b/lib/source/pl/core/evaluator.cpp index 647808d2..6c162e51 100644 --- a/lib/source/pl/core/evaluator.cpp +++ b/lib/source/pl/core/evaluator.cpp @@ -714,8 +714,8 @@ namespace pl::core { auto getStorage = [&, this]() -> auto& { if (heapSection) { - if (auto &heap = this->getHeap(); heap.size() > pattern->getHeapAddress()) - return heap[pattern->getHeapAddress()]; + if (this->getHeap().size() > pattern->getHeapAddress()) + return this->getHeapCellForWriting(pattern->getHeapAddress()); else err::E0011.throwError(fmt::format("Tried accessing out of bounds heap cell {}. This is a bug.", pattern->getHeapAddress())); } else if (patternLocalSection) { @@ -886,6 +886,32 @@ namespace pl::core { this->m_scopes.pop_back(); } + void Evaluator::pushHeapGuard() { + this->m_heapGuards.push_back({ this->m_heap.size(), {} }); + } + + void Evaluator::popHeapGuard() { + if (this->m_heapGuards.empty()) + return; + + auto guard = std::move(this->m_heapGuards.back()); + this->m_heapGuards.pop_back(); + + this->m_heap.resize(guard.heapSize); + for (auto &[address, cell] : guard.originalCells) + this->m_heap[address] = std::move(cell); + } + + std::vector &Evaluator::getHeapCellForWriting(u64 address) { + // Only the innermost guard needs a copy, since it restores the cell before outer guards can see it + if (!this->m_heapGuards.empty()) { + if (auto &guard = this->m_heapGuards.back(); address < guard.heapSize) + guard.originalCells.try_emplace(address, this->m_heap[address]); + } + + return this->m_heap[address]; + } + std::vector unpackCompoundStatements(const std::vector> &nodes) { std::vector result; @@ -919,7 +945,8 @@ namespace pl::core { auto heapAddress = (address >> 32); auto storageAddress = address & 0xFFFF'FFFF; if (heapAddress < heap.size()) { - auto &storage = heap[heapAddress]; + const bool modifiesCell = write || storageAddress + size > heap[heapAddress].size(); + auto &storage = modifiesCell ? this->getHeapCellForWriting(heapAddress) : heap[heapAddress]; if (storageAddress + size > storage.size()) { storage.resize(storageAddress + size); @@ -1021,7 +1048,7 @@ namespace pl::core { if (id == ptrn::Pattern::MainSectionId) err::E0011.throwError("Cannot access main section."); else if (id == ptrn::Pattern::HeapSectionId) - return this->m_heap.back(); + return this->getHeapCellForWriting(this->m_heap.size() - 1); else if (this->m_sections.contains(id)) return this->m_sections[id].data; else if (id == ptrn::Pattern::InstantiationSectionId) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 292b15f5..a62189b4 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -40,6 +40,8 @@ set(AVAILABLE_TESTS Pragmas PragmasFail Format + FormatHeapGrowth + FormatHeapIsolation RValuesAssignmentInStruct TemplateParametersScope TypeNameOf @@ -100,6 +102,7 @@ set(AVAILABLE_TESTS add_executable(pattern_language_tests source/main.cpp source/tests.cpp + source/test_pattern_format_heap.cpp ) diff --git a/tests/include/test_patterns/test_pattern_format_heap.hpp b/tests/include/test_patterns/test_pattern_format_heap.hpp new file mode 100644 index 00000000..8cd9ac35 --- /dev/null +++ b/tests/include/test_patterns/test_pattern_format_heap.hpp @@ -0,0 +1,144 @@ +#pragma once + +#include "test_pattern.hpp" + +#include + +#include +#include + +namespace pl::test { + + /** + * @brief Finds a top-level pattern by its variable name + * @param patterns Patterns to search + * @param name Variable name to look for + * @return The pattern, or nullptr if no pattern has that name + */ + std::shared_ptr findPattern(const std::vector> &patterns, const std::string &name); + + /** + * @brief Calls a function on every pattern inside a pattern that has no + * entries of its own, such as the members of each struct in an array + * @param pattern Pattern to walk through + * @param callback Function to call on each of these patterns + */ + void forEachLeaf(const std::shared_ptr &pattern, const std::function &callback); + + class TestPatternFormatHeapGrowth : public TestPattern { + public: + TestPatternFormatHeapGrowth(core::Evaluator *evaluator) : TestPattern(evaluator, "FormatHeapGrowth") { + } + ~TestPatternFormatHeapGrowth() override = default; + + [[nodiscard]] std::string getSourceCode() const override { + return R"( + str filler = "0123456789ABCDEF" * 0x100000; + + fn format_entry(auto value) { + return "x"; + }; + + struct Entry { + u8 value [[format("format_entry")]]; + }; + + Entry entries[0x4000] @ 0x00; + )"; + } + + [[nodiscard]] bool runChecks(const std::vector> &patterns) const override { + auto entries = findPattern(patterns, "entries"); + if (entries == nullptr) + return false; + + // Each formatter call used to copy the whole heap, which made this take about 20 seconds + const auto start = std::chrono::steady_clock::now(); + + bool valid = true; + forEachLeaf(entries, [&](ptrn::Pattern &pattern) { + if (pattern.getFormattedValue() != "x") + valid = false; + }); + + const auto elapsed = std::chrono::duration_cast(std::chrono::steady_clock::now() - start); + if (elapsed > std::chrono::seconds(5)) { + fmt::print("Formatting took {} ms\n", elapsed.count()); + return false; + } + + return valid; + } + }; + + class TestPatternFormatHeapIsolation : public TestPattern { + public: + TestPatternFormatHeapIsolation(core::Evaluator *evaluator) : TestPattern(evaluator, "FormatHeapIsolation") { + } + ~TestPatternFormatHeapIsolation() override = default; + + [[nodiscard]] std::string getSourceCode() const override { + return R"( + u32 counter = 0; + str label = "a"; + + fn bump_format(auto value) { + counter = counter + 1; + label = "b"; + return "bumped"; + }; + + fn bump_transform(auto value) { + counter = counter + 100; + label = "c"; + return value; + }; + + fn show_state(auto value) { + if (counter == 0 && label == "a") + return "clean"; + else + return "leaked"; + }; + + struct Formatted { + u8 value [[format("bump_format")]]; + }; + + struct Transformed { + u8 value [[transform("bump_transform")]]; + }; + + Formatted formatted[4] @ 0x00; + Transformed transformed[4] @ 0x10; + u8 state @ 0x20 [[format("show_state")]]; + )"; + } + + [[nodiscard]] bool runChecks(const std::vector> &patterns) const override { + auto formatted = findPattern(patterns, "formatted"); + auto transformed = findPattern(patterns, "transformed"); + auto state = findPattern(patterns, "state"); + if (formatted == nullptr || transformed == nullptr || state == nullptr) + return false; + + bool valid = true; + forEachLeaf(formatted, [&](ptrn::Pattern &pattern) { + if (pattern.getFormattedValue() != "bumped") + valid = false; + }); + forEachLeaf(transformed, [&](ptrn::Pattern &pattern) { + wolv::util::unused(pattern.getValue()); + }); + + // Changes that formatters and transforms make to globals must not last after they return + if (auto stateValue = state->getFormattedValue(); stateValue != "clean") { + fmt::print("Formatter state leaked: {}\n", stateValue); + valid = false; + } + + return valid; + } + }; + +} diff --git a/tests/source/test_pattern_format_heap.cpp b/tests/source/test_pattern_format_heap.cpp new file mode 100644 index 00000000..a443d110 --- /dev/null +++ b/tests/source/test_pattern_format_heap.cpp @@ -0,0 +1,24 @@ +#include "test_patterns/test_pattern_format_heap.hpp" + +namespace pl::test { + + std::shared_ptr findPattern(const std::vector> &patterns, const std::string &name) { + for (const auto &pattern : patterns) { + if (pattern->getVariableName() == name) + return pattern; + } + + return nullptr; + } + + void forEachLeaf(const std::shared_ptr &pattern, const std::function &callback) { + if (auto iterable = dynamic_cast(pattern.get()); iterable != nullptr) { + iterable->forEachEntry(0, iterable->getEntryCount(), [&](u64, const auto &entry) { + forEachLeaf(entry, callback); + }); + } else { + callback(*pattern); + } + } + +} diff --git a/tests/source/tests.cpp b/tests/source/tests.cpp index 0e6510ef..0bd08ef2 100644 --- a/tests/source/tests.cpp +++ b/tests/source/tests.cpp @@ -28,6 +28,7 @@ #include "test_patterns/test_pattern_pragmas_pr249.hpp" #include "test_patterns/test_pattern_pragmas_fail.hpp" #include "test_patterns/test_pattern_format.hpp" +#include "test_patterns/test_pattern_format_heap.hpp" #include "test_patterns/test_pattern_rvalues_assignment_in_struct.hpp" #include "test_patterns/test_pattern_template_parameters_scope.hpp" #include "test_patterns/test_pattern_typenameof.hpp" @@ -83,6 +84,8 @@ std::array Tests = { TEST(PragmasIssue249), TEST(PragmasFail), TEST(Format), + TEST(FormatHeapGrowth), + TEST(FormatHeapIsolation), TEST(RValuesAssignmentInStruct), TEST(TemplateParametersScope), TEST(TypeNameOf),