mirror of
https://github.com/sbrow/thor.git
synced 2026-10-25 02:01:56 -04:00
chore: Added docs for how to deal with multi-threaded tree-sitter.
This commit is contained in:
@@ -0,0 +1,148 @@
|
||||
# Grammar cache thread-safety & allocator lifetime
|
||||
|
||||
Status: **investigation complete, fix deferred.** This document records what we
|
||||
found and the direction we chose, so the next person doesn't have to rediscover
|
||||
it. No behavior has been changed yet — only documentation comments were added to
|
||||
`treesitter/treesitter.odin`.
|
||||
|
||||
## How we got here
|
||||
|
||||
While adding a feature to minify inline `<style>` blocks (route the CSS body
|
||||
through `minify_css`), the new unit tests exercised `minify_html` / `minify_css`
|
||||
for the first time. Those are the first tests to touch the tree-sitter parser
|
||||
cache, and under the multi-threaded test runner (default 16 threads) they failed
|
||||
**intermittently** with a rotating cast of symptoms:
|
||||
|
||||
- `panic: unable to insert into a map`
|
||||
- tree-sitter internal aborts: `ts_subtree_retain: Assertion 'self.ptr->ref_count > 0' failed`, `_array__erase: Assertion 'index < *size' failed`
|
||||
- `malloc(): unaligned tcache chunk detected`
|
||||
- assertions passing on the wrong data (e.g. minified output missing `<style>`)
|
||||
|
||||
The flakiness comes from **three distinct problems**, two thread-safety and one
|
||||
allocator-lifetime. They are orthogonal — fixing one does not fix the others.
|
||||
|
||||
## The three issues
|
||||
|
||||
### #1a — concurrent cache **writes** (data race)
|
||||
|
||||
`ensure_parser` (`treesitter.odin`) reads and writes `grammar_store.cache`
|
||||
with **no lock**. `cache_mutex` exists but is only taken on the parallel preload
|
||||
path (`grammar_worker`), never in the lazy `ensure_parser` / `load_grammar`
|
||||
paths. Two threads first-touching `"html"`/`"css"` at once corrupt the map.
|
||||
|
||||
### #1b — concurrent parser **use** (data race)
|
||||
|
||||
There is exactly **one cached `Grammar_Cache` (and one `parser`) per language**.
|
||||
When two threads both call `minify_html`, they fetch the *same* `gc.parser` and
|
||||
call `parser_parse_string` on it simultaneously. A tree-sitter `TSParser`
|
||||
mutates its internal subtree pool during a parse, so concurrent parses corrupt
|
||||
it — that's the `ts_subtree_retain` / `_array__erase` aborts.
|
||||
|
||||
This is **not fixed by preloading/"loading html+css statically."** Loading gives
|
||||
you *one shared* parser; the race is about *using* that one parser from two
|
||||
threads. It is fixed only by serializing the parse or giving each thread its own
|
||||
parser.
|
||||
|
||||
### #2 — allocator lifetime (not a race)
|
||||
|
||||
`init_persistent` captures `context.allocator` into the process-lifetime
|
||||
`grammar_store`. In `main` that happens to be the heap (it runs before the site
|
||||
arena is installed at `main.odin`), so production is fine. But whatever
|
||||
allocator is live at that instant becomes the cache's owner forever:
|
||||
|
||||
- Under the **test runner**, `context.allocator` is a **per-test tracking
|
||||
allocator** that is torn down when that test ends → the global cache is left
|
||||
pointing at dead memory (→ `unable to insert into a map`, garbage reads).
|
||||
- A **lazy** first call during rendering would capture the **site arena**
|
||||
(`context.allocator` after `main.odin` swaps it in), which is destroyed every
|
||||
watch-mode rebuild → dangling parsers across builds.
|
||||
|
||||
This would break even single-threaded; threading just decides *which* test wins
|
||||
the init race.
|
||||
|
||||
## tree-sitter's threading model (the root fact)
|
||||
|
||||
tree-sitter objects have specific, documented threading rules:
|
||||
|
||||
| Object | Shareable across threads? | Rule |
|
||||
|---|---|---|
|
||||
| `TSLanguage` (`tree_sitter_html()` etc.) | Yes | Immutable; share freely. |
|
||||
| `TSParser` (`gc.parser`) | **No** | One per thread; never parse on one parser from two threads. |
|
||||
| `TSTree` | Only via `ts_tree_copy` | Copy before using on another thread. |
|
||||
| `TSQuery` (`gc.query`) | Yes | Immutable after compile. |
|
||||
| `TSQueryCursor` (`gc.cursor`) | **No** | One per thread, like the parser. |
|
||||
|
||||
This explains the existing design:
|
||||
|
||||
- The preload fan-out (`preload_grammar` / `grammar_worker`) creates a parser
|
||||
**per worker** and only publishes the finished `Grammar_Cache` under
|
||||
`cache_mutex` — because parsers can't be shared during construction/use.
|
||||
- `ensure_parser` + the shared cached parser is fine in production **only
|
||||
because rendering is single-threaded**.
|
||||
|
||||
A cached `Grammar_Cache` mixes shareable (`language`, `query`) and per-thread
|
||||
(`parser`, `cursor`) objects in one shared struct, which makes it
|
||||
single-threaded-use by construction — a fact that was previously undocumented.
|
||||
|
||||
## Where this actually bites
|
||||
|
||||
**Only in tests.** Confirmed by grep:
|
||||
|
||||
- Production parses on one thread: every real caller — `minify.odin` (html/css)
|
||||
and `markdown/highlight.odin` — runs inside `main`'s single-threaded render
|
||||
loop. No concurrent `ensure_parser` / `parser_parse_string`.
|
||||
- The one threaded path in production, `preload_grammars`, is already safe (own
|
||||
parser per worker + `cache_mutex` on the write) and only handles non-builtin
|
||||
grammars.
|
||||
- Only `minify_test.odin` touches tree-sitter from tests, and the test runner
|
||||
runs those on many threads.
|
||||
|
||||
So production is correct **by convention** (single-threaded-from-heap usage),
|
||||
not **by construction** — nothing stops a future parallel render from breaking
|
||||
it, and until now nothing documented the constraint.
|
||||
|
||||
## Chosen direction
|
||||
|
||||
**For now:** keep the single global grammar cache and make it safe by
|
||||
**serializing the non-thread-safe behavior with a mutex** — only one thread uses
|
||||
a parser at a time. This is simple, matches how production already behaves, and
|
||||
unblocks the tests.
|
||||
|
||||
**Later (TODO):** decide whether true multi-threaded parsing is worth it for
|
||||
performance. If yes, give each thread its own parser + cursor (as the preload
|
||||
workers already do) rather than sharing one behind a lock.
|
||||
|
||||
## Implementation options (deferred — pick when we resume)
|
||||
|
||||
All assume the allocator fix (#2) below, which is independent and unambiguous.
|
||||
|
||||
- **#2 allocator (do regardless):** pin the heap allocator explicitly —
|
||||
`grammar_store.allocator = runtime.heap_allocator()` (or `os.heap_allocator()`)
|
||||
instead of `context.allocator`. Makes the cache persistent from any context;
|
||||
behavior-identical in production. Optionally make `init_persistent` idempotent
|
||||
and/or an `@(init)` so callers need no ordering.
|
||||
|
||||
Then, for the races:
|
||||
|
||||
- **Option A — library mutex (sustainable):** take `cache_mutex` in
|
||||
`ensure_parser` (fixes #1a) and add a locking `parse` helper that wraps
|
||||
`parser_parse_string` (fixes #1b), wired into the three call sites
|
||||
(`minify.odin` ×2, `markdown/highlight.odin` ×1). Tests then need no
|
||||
scaffolding. Serializes all parsing.
|
||||
|
||||
- **Option B — test-side only (minimal):** leave the library single-threaded by
|
||||
contract; make the tests not violate it. Either fold the minify tests into one
|
||||
`@(test)` (no concurrency at all → no locks), or wrap the `minify_*` calls in a
|
||||
test-local mutex. Production stays lock-free.
|
||||
|
||||
Recommendation captured during the discussion: the allocator one-liner is
|
||||
worth doing immediately; for the races, Option A is the "correct" long-term
|
||||
answer but only necessary if/when we want concurrent parsing — otherwise
|
||||
Option B is enough.
|
||||
|
||||
## Pointers
|
||||
|
||||
- `treesitter/treesitter.odin`: doc comments on `Grammar_Cache`, `cache_mutex`,
|
||||
`init_persistent`, `ensure_parser` reference this file.
|
||||
- Original feature that surfaced this: minify inline `<style>` bodies via
|
||||
`minify_css` (reverted; to be re-landed alongside whichever fix we choose).
|
||||
@@ -2,6 +2,13 @@
|
||||
|
||||
## Priority Unclear (Evaluate Later)
|
||||
|
||||
- [ ] Decide whether the grammar cache should support multi-threaded parsing.
|
||||
- For now it stays a single global cache used single-threaded; the plan is to
|
||||
guard the non-thread-safe parser/cursor use with a mutex when tests (or any
|
||||
future parallel caller) need it. A `TSParser`/`TSQueryCursor` is one-per-thread.
|
||||
- Alternative later: give each thread its own parser+cursor for real parallelism.
|
||||
- Also pin `init_persistent` to the heap allocator (lifetime bug, independent).
|
||||
- Full write-up: `GRAMMAR_CACHE_THREADING.md`.
|
||||
- [ ] instead of `warnings: [dynamic]Error` we should use `warnings: [dynamic; 8]Error`
|
||||
- when limit reached, the template fails and stops rendering.
|
||||
- [ ] Brainstorm plugin/tool/(postccs/less/tailwind/sass) support.
|
||||
|
||||
@@ -120,6 +120,12 @@ foreign css_grammar {
|
||||
tree_sitter_css :: proc() -> Language ---
|
||||
}
|
||||
|
||||
// THREAD-SAFETY: a Grammar_Cache bundles shareable tree-sitter objects
|
||||
// (`language`, `query` — immutable, safe to read from many threads) with
|
||||
// per-thread ones (`parser`, `cursor` — a TSParser mutates its internal
|
||||
// subtree pool during a parse, and a TSQueryCursor is likewise stateful).
|
||||
// A cached Grammar_Cache is therefore SINGLE-THREADED-USE: exactly one thread
|
||||
// may drive `parser`/`cursor` at a time. See GRAMMAR_CACHE_THREADING.md.
|
||||
Grammar_Cache :: struct {
|
||||
language: Language,
|
||||
parser: Parser,
|
||||
@@ -133,6 +139,12 @@ Get_Language_Proc :: #type proc() -> Language
|
||||
SPALL :: #config(SPALL, false)
|
||||
|
||||
grammar_store: Grammar_Store
|
||||
|
||||
// cache_mutex guards writes to grammar_store.cache. NOTE: today it is only
|
||||
// taken on the parallel preload path (grammar_worker), NOT in ensure_parser /
|
||||
// load_grammar. Those lazy paths are intentionally unlocked because production
|
||||
// only ever calls them single-threaded. See GRAMMAR_CACHE_THREADING.md before
|
||||
// relying on this cache from multiple threads.
|
||||
cache_mutex: sync.Mutex
|
||||
|
||||
Grammar_Store :: struct {
|
||||
@@ -140,6 +152,14 @@ Grammar_Store :: struct {
|
||||
allocator: mem.Allocator,
|
||||
}
|
||||
|
||||
// KNOWN ISSUE (allocator lifetime): grammar_store is a process-lifetime global,
|
||||
// but this captures whatever `context.allocator` is live at call time. In main
|
||||
// that is deliberately the heap (init_persistent runs before the site arena is
|
||||
// installed), so it works. But it is fragile: called from any other context
|
||||
// (e.g. a per-test tracking allocator, or lazily once the site arena is active)
|
||||
// it would bind the cache to a transient allocator and leave it dangling once
|
||||
// that allocator is torn down. The intended fix is to pin the heap allocator
|
||||
// explicitly (runtime.heap_allocator()). See GRAMMAR_CACHE_THREADING.md.
|
||||
init_persistent :: proc() {
|
||||
grammar_store.allocator = context.allocator
|
||||
grammar_store.cache = make(map[string]^Grammar_Cache, grammar_store.allocator)
|
||||
@@ -219,6 +239,12 @@ load_language :: proc(lang: string) -> (language: Language, ok: bool) {
|
||||
return
|
||||
}
|
||||
|
||||
// THREAD-SAFETY: ensure_parser is NOT safe to call concurrently. It reads and
|
||||
// writes grammar_store.cache without taking cache_mutex, and it returns a shared
|
||||
// per-language Grammar_Cache whose `parser` is not reentrant. It is written for
|
||||
// single-threaded lazy use (minify, syntax highlighting). Callers that need
|
||||
// parsing on multiple threads must serialize with a lock or give each thread
|
||||
// its own parser. See GRAMMAR_CACHE_THREADING.md.
|
||||
ensure_parser :: proc(lang: string) -> ^Grammar_Cache {
|
||||
if cached, ok := grammar_store.cache[lang]; ok {
|
||||
return cached
|
||||
|
||||
Reference in New Issue
Block a user