Considerably more search internals #9585
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Chugging along towards #189
This adds significantly more internal work for searching. A long time ago, I added #2885 which had a hint of what I was thinking of. This simultaneously builds on this and changes direction.
The change of direction is that instead of making PageList fully concurrency safe and having a search thread access it concurrently, I'm now making an architectural shift where our search thread will grab the big lock (blocking all IO/rendering), but with the bet that we can make our critical areas small enough and time them well enough that the performance hit while actively searching will be minimal. Results yet to be seen, but the path to implement this is much, much simpler.
Rearchitecting Search
To that end, this PR builds on #2885 by making
src/terminal/searchand entire package (rather than a single file).graph TB subgraph Layer5 ["<b>Layer 5: Thread Orchestration</b>"] Thread["<b>Thread</b><br/>━━━━━━━━━━━━━━━━━━━━━<br/>• MPSC queue management<br/>• libxev event loop<br/>• Message handling<br/>• Surface mailbox communication<br/>• Forward progress coordination"] end subgraph Layer4 ["<b>Layer 4: Screen Coordination</b>"] ScreenSearch["<b>ScreenSearch</b><br/>━━━━━━━━━━━━━━━━━━━━━<br/>• State machine (tick + feed)<br/>• Result caching<br/>• Per-screen (alt/primary)<br/>• Composes Active + History search<br/>• Interrupt handling"] end subgraph Layer3 ["<b>Layer 3: Domain-Specific Search</b>"] ActiveSearch["<b>ActiveSearch</b><br/>━━━━━━━━━━━━━━━━━━━━━<br/>• Active area only<br/>• Invalidate & re-search<br/>• Small, volatile data"] PageListSearch["<b>PageListSearch</b><br/>━━━━━━━━━━━━━━━━━━━━━<br/>• History search (reverse order)<br/>• Separated tick/feed ops<br/>• Immutable PageList assumption<br/>• Garbage pin detection"] end subgraph Layer2 ["<b>Layer 1: Primitive Operations</b>"] SlidingWindow["<b>SlidingWindow</b><br/>━━━━━━━━━━━━━━━━━━━━━<br/>• Manual linked list node management<br/>• Circular buffer maintenance<br/>• Zero-allocation search<br/>• Match yielding<br/>• Page boundary handling"] end Thread --> ScreenSearch ScreenSearch --> ActiveSearch ScreenSearch --> PageListSearch ActiveSearch --> SlidingWindow PageListSearch --> SlidingWindow classDef layer5 fill:#0a0a0a,stroke:#ff0066,stroke-width:3px,color:#ffffff classDef layer4 fill:#0f0f0f,stroke:#ff6600,stroke-width:3px,color:#ffffff classDef layer3 fill:#141414,stroke:#ffaa00,stroke-width:3px,color:#ffffff classDef layer2 fill:#1a1a1a,stroke:#00ff00,stroke-width:3px,color:#ffffff class Thread layer5 class ScreenSearch layer4 class ActiveSearch,PageListSearch layer3 class SlidingWindow layer2 style Layer5 fill:#050505,stroke:#ff0066,stroke-width:2px,color:#ffffff style Layer4 fill:#080808,stroke:#ff6600,stroke-width:2px,color:#ffffff style Layer3 fill:#0c0c0c,stroke:#ffaa00,stroke-width:2px,color:#ffffff style Layer2 fill:#101010,stroke:#00ff00,stroke-width:2px,color:#ffffffWithin the package, we have composable layers that let us test each point:
SlidingWindow: The lowest layer, the caller manually adds linked list page nodes and it maintains a sliding window we search over, yielding results without allocation (besides the circular buffers to maintain the sliding window).PageListSearch: Searches a PageList structure in reverse order (assumption: more recent matches are more valuable than older), but separates out thetick(search, but no PageList access) andfeed(PageList access, prep data for search but don't search) operations. This lets usfeedin a critical area andtickoutside. This assumes an immutable PageList, so this is for history.ActiveSearch: Searches only the active area of a PageList. The expectation is that the active area changes much more regularly, but it is also very small (relative to scrollback). Throws away and re-searches the active area as necessary.ScreenSearch: Composes the previous three components to coordinate searching an active terminal screen. You'd have one of these per screen (alt vs primary). This also caches results unlike the other components, with the expectation that the caller will revisit the results as screens change (so if you switch from neovim back to your shell and vice versa with a search active, it won't start over).Thread: A dedicated search thread that will receive messages via MPSC queues while managing the forward progress of aScreenSearchand sending matches back to the surface mailbox for apprt rendering. The thread component is not functional, just boilerplate, in this PR.ScreenSearch is a state machine that moves in an iterative
tick+feedfashion. This will let us "interrupt" the search with updates on the search thread (read our mailbox via libxev loops for example) and will let us minimize critical areas with locks (onlyfeed).Each component is significantly unit tested, especially around page boundary cases. Given the complexity, there is no way this is perfect, but the architecture is such that we can easily add regression tests as we find issues.
Other Changes, Notes
The only change to actually used code is that tracked pins in a
PageListcan now be flagged as "garbage." A garbage tracked pin is one that had to be moved in a non-sensical way because the previous location it tracked has been deleted. This is used by the searcher to detect that our history was pruned.If my assumption about the big lock is wrong and this ends up being godawful for performance, then it should still be okay because more granular locking and reference counting such as that down by @dave-fl in #8850 can be pushed into these components and reused. So this work is still valuable on its own.
Future
This PR is still just a bunch of internals, split out into its own PR so I don't make one huge 10K diff PR. There are a number of future tasks:
ScreenSearchand hook it up toThreadSurface(or possibly the render thread or shared render state since active area changes can be synchronized with renderer frame rebuilds. Not sure yet.)The next step is to continue to flesh out the
ScreenSearchas required and hook it up toThread.AI disclosure: AI reviewed the code and assisted with some tests, but didn't write any of the logic or design. This is beyond its ability (or my ability to spec it out clearly enough for AI to succeed).