From f853fa60b15f549bfc0a6d02a0747973c115c2cc Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 03:53:53 +0000 Subject: [PATCH] Highlighting: invalidate from the edited row, not the whole buffer Every edit bumped the buffer version (through SetDirty) and called InvalidateFrom(0), so all cached spans and line states were discarded and stateful highlighters re-lexed from row 0 to the viewport on each keystroke. Buffer mutators now report the row they changed (edited_at_), which bumps the version and tells the engine to drop only rows from there down; a row's highlighting depends only on rows above it. A version change that arrives without that notice still clears everything, so a missed path cannot show stale spans. The engine keeps states as a contiguous vector and caches spans for every row it lexes. SetDirty no longer touches the version: the edits themselves do, including undo and redo back to the saved state (D9 still holds; its test now checks the highlighting itself). Typing near the end of a 100k-line C++ file: 46.3 ms -> 0.14 ms of highlighting work per keystroke. Audit finding P3. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB --- Buffer.cc | 6 ++ Buffer.h | 23 +++-- syntax/HighlighterEngine.cc | 173 ++++++++++++-------------------- syntax/HighlighterEngine.h | 31 +++--- tests/test_audit_regressions.cc | 79 +++++++++++++-- 5 files changed, 177 insertions(+), 135 deletions(-) diff --git a/Buffer.cc b/Buffer.cc index 9d8f6b5..efdef54 100644 --- a/Buffer.cc +++ b/Buffer.cc @@ -628,6 +628,7 @@ Buffer::insert_text(int row, int col, std::string_view text) if (!text.empty()) { content_.Insert(off, text.data(), text.size()); rows_cache_dirty_ = true; + edited_at_(row); if (swap_rec_) swap_rec_->OnInsert(row, col, text); } @@ -709,6 +710,7 @@ Buffer::delete_text(int row, int col, std::size_t len) return; content_.Delete(start, actual); rows_cache_dirty_ = true; + edited_at_(row); if (swap_rec_) swap_rec_->OnDelete(row, col, actual); } @@ -727,6 +729,7 @@ Buffer::split_line(int row, const int col) const char nl = '\n'; content_.Insert(off, &nl, 1); rows_cache_dirty_ = true; + edited_at_(row); if (swap_rec_) swap_rec_->OnInsert(row, c, std::string_view("\n", 1)); } @@ -746,6 +749,7 @@ Buffer::join_lines(int row) // end_of_line now equals line end (clamped before newline). The newline should be exactly at this position. content_.Delete(end_of_line, 1); rows_cache_dirty_ = true; + edited_at_(row); if (swap_rec_) swap_rec_->OnDelete(row, col, 1); } @@ -762,6 +766,7 @@ Buffer::insert_row(int row, const std::string_view text) const char nl = '\n'; content_.Insert(off + text.size(), &nl, 1); rows_cache_dirty_ = true; + edited_at_(row); if (swap_rec_) { // Avoid allocation: emit the row text insertion (if any) and the newline insertion. if (!text.empty()) @@ -789,6 +794,7 @@ Buffer::delete_row(int row) return; content_.Delete(start, actual); rows_cache_dirty_ = true; + edited_at_(row); if (swap_rec_) swap_rec_->OnDelete(row, 0, actual); } diff --git a/Buffer.h b/Buffer.h index c6f177e..ba33ce2 100644 --- a/Buffer.h +++ b/Buffer.h @@ -390,21 +390,17 @@ public: void SetDirty(bool d) { + // The version tracks content, not the dirty flag: every text mutation + // bumps it (see edited_at_), including undo/redo back to the saved state. dirty_ = d; - // Bump even when clearing: undo/redo back to the saved state changes the - // text and then calls SetDirty(false). Without a bump the highlighter - // keeps serving spans for the pre-undo text. - MarkContentChanged(); } - // Invalidate version-keyed caches (syntax highlighting) after a content change. + // Whole-buffer content change (load, reload, replace): bump the version + // and drop all cached highlighting. void MarkContentChanged() { - ++version_; - if (highlighter_) { - highlighter_->InvalidateFrom(0); - } + edited_at_(0); } @@ -699,6 +695,15 @@ private: // Syntax/highlighting state std::uint64_t version_ = 0; // increment on edits + + // Record a content change starting at `row`: bump the version and let the + // highlighter drop cached rows from `row` down (rows above are unaffected). + void edited_at_(int row) + { + ++version_; + if (highlighter_) + highlighter_->OnEdit(row < 0 ? 0 : row, version_); + } bool syntax_enabled_ = true; bool syntax_user_override_ = false; // true once user explicitly set syntax state via a command std::string filetype_; diff --git a/syntax/HighlighterEngine.cc b/syntax/HighlighterEngine.cc index 6cfab06..206f894 100644 --- a/syntax/HighlighterEngine.cc +++ b/syntax/HighlighterEngine.cc @@ -2,6 +2,8 @@ #include "../Buffer.h" #include "LanguageHighlighter.h" +#include + namespace kte { HighlighterEngine::HighlighterEngine() = default; @@ -14,109 +16,79 @@ HighlighterEngine::SetHighlighter(std::unique_ptr hl) { std::lock_guard lock(mtx_); hl_ = std::move(hl); + clear_caches_locked(); +} + + +void +HighlighterEngine::clear_caches_locked() const +{ cache_.clear(); - state_cache_.clear(); - state_last_contig_.clear(); + states_.clear(); +} + + +void +HighlighterEngine::invalidate_from_locked(int row) const +{ + if (row < 0) + row = 0; + cache_.erase(cache_.lower_bound(row), cache_.end()); + if (states_.size() > static_cast(row)) + states_.resize(static_cast(row)); } LineHighlight HighlighterEngine::GetLine(const Buffer &buf, int row, std::uint64_t buf_version) const { - std::unique_lock lock(mtx_); - auto it = cache_.find(row); - if (it != cache_.end() && it->second.version == buf_version) { - return it->second; // return by value (copy) + std::lock_guard lock(mtx_); + if (!have_version_ || buf_version != version_) { + // Content changed without OnEdit: nothing cached can be trusted. + clear_caches_locked(); + version_ = buf_version; + have_version_ = true; } + auto it = cache_.find(row); + if (it != cache_.end()) + return it->second; // return by value (copy) - // We'll compute into a local result to avoid exposing references to cache LineHighlight result; result.version = buf_version; - result.spans.clear(); - if (!hl_) { - // Cache empty result and return it + if (!hl_ || row < 0) { cache_[row] = result; return result; } - // Copy shared_ptr-like raw pointer for use outside critical sections - LanguageHighlighter *hl_ptr = hl_.get(); - bool is_stateful = dynamic_cast(hl_ptr) != nullptr; - - if (!is_stateful) { - // Stateless fast path: we can release the lock while computing to reduce contention - lock.unlock(); - hl_ptr->HighlightLine(buf, row, result.spans); - // Update cache and return - std::lock_guard gl(mtx_); + auto *stateful = dynamic_cast(hl_.get()); + if (!stateful) { + hl_->HighlightLine(buf, row, result.spans); cache_[row] = result; return result; } - // Stateful path: we need to walk from a known previous state. Keep lock while consulting caches, - // but release during heavy computation. - auto *stateful = static_cast(hl_ptr); - - StatefulHighlighter::LineState prev_state; - int start_row = -1; - - // Fast path: state_last_contig_ tracks the highest row we've cached state - // for, per version. If that row is already below our target, it's - // necessarily the best anchor the O(n) scan below would have found, so we - // can skip the scan entirely. Re-validated against state_cache_ before - // use since InvalidateFrom()/SetHighlighter() may have raced/cleared it. - auto contig_it = state_last_contig_.find(buf_version); - if (contig_it != state_last_contig_.end() && contig_it->second < row) { - auto sc_it = state_cache_.find(contig_it->second); - if (sc_it != state_cache_.end() && sc_it->second.version == buf_version) { - start_row = contig_it->second; - prev_state = sc_it->second.state; - } + // Stateful: continue from the last row with a known end state (or the + // row just above the target, if that is known), caching every row's + // spans and state along the way so neighbouring rows are cache hits. + const int known = static_cast(states_.size()) - 1; + const int start = std::min(known, row - 1); + StatefulHighlighter::LineState state = (start >= 0) ? states_[static_cast(start)] + : StatefulHighlighter::LineState{}; + const int nrows = static_cast(buf.Nrows()); + for (int r = start + 1; r <= row && r < nrows; ++r) { + LineHighlight lh; + lh.version = buf_version; + state = stateful->HighlightLineStateful(buf, r, state, lh.spans); + if (static_cast(r) == states_.size()) + states_.push_back(state); + else + states_[static_cast(r)] = state; + if (r == row) + result = lh; + else + cache_.insert_or_assign(r, std::move(lh)); } - - if (start_row < 0 && !state_cache_.empty()) { - // linear search over map (unordered), track best candidate - int best = -1; - for (const auto &kv: state_cache_) { - int r = kv.first; - // Only use cached state if it's for the current version and row still exists - if (r <= row - 1 && kv.second.version == buf_version) { - // Validate that the cached row index is still valid in the buffer - if (r >= 0 && static_cast(r) < buf.Nrows()) { - if (r > best) - best = r; - } - } - } - if (best >= 0) { - start_row = best; - prev_state = state_cache_.at(best).state; - } - } - - // We'll compute states and the target line's spans without holding the lock for most of the work. - // Create a local copy of prev_state and iterate rows; we will update caches under lock. - lock.unlock(); - StatefulHighlighter::LineState cur_state = prev_state; - for (int r = start_row + 1; r <= row; ++r) { - std::vector tmp; - std::vector &out = (r == row) ? result.spans : tmp; - auto next_state = stateful->HighlightLineStateful(buf, r, cur_state, out); - // Update state cache for r - std::lock_guard gl(mtx_); - StateEntry se; - se.version = buf_version; - se.state = next_state; - state_cache_[r] = se; - cur_state = next_state; - int &contig = state_last_contig_[buf_version]; - if (r > contig) - contig = r; - } - - // Store in cache and return by value - lock.lock(); cache_[row] = result; return result; } @@ -126,31 +98,18 @@ void HighlighterEngine::InvalidateFrom(int row) { std::lock_guard lock(mtx_); - if (cache_.empty()) - return; - // Simple implementation: erase all rows >= row - for (auto it = cache_.begin(); it != cache_.end();) { - if (it->first >= row) - it = cache_.erase(it); - else - ++it; - } - if (!state_cache_.empty()) { - for (auto it = state_cache_.begin(); it != state_cache_.end();) { - if (it->first >= row) - it = state_cache_.erase(it); - else - ++it; - } - } - // A version's tracked contiguous-state row is no longer valid once any - // row at or above it has been evicted from state_cache_ above. - for (auto it = state_last_contig_.begin(); it != state_last_contig_.end();) { - if (it->second >= row) - it = state_last_contig_.erase(it); - else - ++it; - } + invalidate_from_locked(row); +} + + +void +HighlighterEngine::OnEdit(int row, std::uint64_t buf_version) +{ + std::lock_guard lock(mtx_); + if (have_version_) + invalidate_from_locked(row); + version_ = buf_version; + have_version_ = true; } diff --git a/syntax/HighlighterEngine.h b/syntax/HighlighterEngine.h index 8e581a9..5c5e164 100644 --- a/syntax/HighlighterEngine.h +++ b/syntax/HighlighterEngine.h @@ -2,8 +2,8 @@ #pragma once #include +#include #include -#include #include #include @@ -29,6 +29,12 @@ public: // Invalidate cached lines from row (inclusive) void InvalidateFrom(int row); + // The buffer changed at `row` and is now at `buf_version`. Highlighting + // of a row depends only on the rows above it, so rows before `row` stay + // cached. A version change that arrives without OnEdit (a path that + // changed content without reporting it) still clears the whole cache. + void OnEdit(int row, std::uint64_t buf_version); + bool HasHighlighter() const { @@ -46,20 +52,21 @@ public: private: std::unique_ptr hl_; - // Simple cache by row index (mutable to allow caching in const GetLine) - mutable std::unordered_map cache_; + // Cached spans by row, valid for version_ (ordered so InvalidateFrom can + // drop a tail cheaply). + mutable std::map cache_; - // For stateful highlighters, remember per-line state (state after finishing that row) - struct StateEntry { - std::uint64_t version{0}; - // Using the interface type; forward-declare via header - StatefulHighlighter::LineState state; - }; + // For stateful highlighters: states_[r] is the state after row r, for + // every row 0..states_.size()-1 (always a contiguous prefix). + mutable std::vector states_; - mutable std::unordered_map state_cache_; + // Buffer version the caches describe. + mutable std::uint64_t version_{0}; + mutable bool have_version_{false}; - // Track best known contiguous state row for a given version to avoid O(n) scans - mutable std::unordered_map state_last_contig_; + void clear_caches_locked() const; + + void invalidate_from_locked(int row) const; // Guards the caches above. mutable std::mutex mtx_; diff --git a/tests/test_audit_regressions.cc b/tests/test_audit_regressions.cc index 4de17cf..c9a25a8 100644 --- a/tests/test_audit_regressions.cc +++ b/tests/test_audit_regressions.cc @@ -9,6 +9,9 @@ #include "ErrorRecovery.h" #include "PieceTable.h" #include "UndoTree.h" +#include "syntax/HighlighterEngine.h" +#include "syntax/HighlighterRegistry.h" +#include "syntax/LanguageHighlighter.h" #include #include @@ -202,15 +205,77 @@ TEST(Audit_D6_Reflow_OnBlankLine_KeepsParagraphsSeparate) } -// D9: clearing the dirty flag (undo to the saved state) changes the version -// so version-keyed highlight caches are refreshed. -TEST(Audit_D9_SetDirtyFalse_BumpsVersion) +static bool +row_has_comment(Buffer &b, int row) { - Buffer b; - b.SetDirty(true); - const auto v = b.Version(); + const auto lh = b.Highlighter()->GetLine(b, row, b.Version()); + for (const auto &sp: lh.spans) + if (sp.kind == kte::TokenKind::Comment) + return true; + return false; +} + + +// D9: undo back to the saved state must not leave stale highlighting (the +// rest of the file coloured as a comment after "/*" is undone). +TEST(Audit_D9_UndoToSaved_RefreshesHighlighting) +{ + TestHarness h; + Buffer &b = h.Buf(); + b.insert_text(0, 0, "int a;\nint b;\n"); + b.EnsureHighlighter(); + b.Highlighter()->SetHighlighter(kte::HighlighterRegistry::CreateFor("cpp")); + if (auto *u = b.Undo()) + u->mark_saved(); b.SetDirty(false); - ASSERT_TRUE(b.Version() != v); + ASSERT_TRUE(!row_has_comment(b, 1)); + + b.SetCursor(0, 0); + ASSERT_TRUE(h.Exec(CommandId::InsertText, "/*")); + ASSERT_TRUE(row_has_comment(b, 1)); + + ASSERT_TRUE(h.Undo()); + ASSERT_EQ(h.Line(0), std::string("int a;")); + ASSERT_TRUE(!b.Dirty()); + ASSERT_TRUE(!row_has_comment(b, 1)); +} + + +// P3: an edit keeps cached highlighting for the rows above it; only rows +// from the edit down are recomputed. +TEST(Audit_P3_EditKeepsHighlightCacheAbove) +{ + struct Counting : kte::StatefulHighlighter { + int *calls; + explicit Counting(int *c) : calls(c) {} + + void HighlightLine(const Buffer &, int, std::vector &) const override {} + + LineState HighlightLineStateful(const Buffer &, int, const LineState &prev, + std::vector &) const override + { + ++*calls; + return prev; + } + }; + + TestHarness h; + Buffer &b = h.Buf(); + std::string text; + for (int i = 0; i < 1000; ++i) + text += "line\n"; + b.insert_text(0, 0, text); + b.EnsureHighlighter(); + int calls = 0; + b.Highlighter()->SetHighlighter(std::make_unique(&calls)); + + (void) b.Highlighter()->GetLine(b, 999, b.Version()); + ASSERT_EQ(calls, 1000); + + calls = 0; + b.insert_text(990, 0, "x"); + (void) b.Highlighter()->GetLine(b, 999, b.Version()); + ASSERT_EQ(calls, 10); // rows 990..999, not the whole file }