diff --git a/GRAMMAR_CACHE_THREADING.md b/GRAMMAR_CACHE_THREADING.md index 228912a..61ad7ef 100644 --- a/GRAMMAR_CACHE_THREADING.md +++ b/GRAMMAR_CACHE_THREADING.md @@ -1,9 +1,32 @@ # 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`. +Status: **resolved.** All three issues below are fixed. The cache was redesigned +so the library caches only the immutable, shareable objects and callers own the +per-thread ones. The rest of this document is kept as the rationale. + +## Resolution (what shipped) + +The `Grammar_Cache` (which mixed shareable `language`/`query` with per-thread +`parser`/`cursor`) was split: + +- **`Grammar`** holds only the immutable `language` + `query`. It lives in a + process-lifetime **`registry`** (`map[string]^Grammar`) guarded by one mutex. +- **`grammar(lang)`** is the thread-safe, self-initializing, lazy loader — fixes + **#1a** (all map access under the lock) and lets tests run with no setup. +- The registry allocator is pinned with `os.heap_allocator()` — fixes **#2**; the + cache no longer captures a transient `context.allocator`. +- Parsers/cursors are **caller-owned**: `open_parser(g)` mints one, the caller + `defer`s `parser_delete` / `query_cursor_delete`. No parser is shared, so + **#1b** is gone. `open_parser` is the seam where a future pool / thread-local + strategy plugs in without touching call sites. +- Deleted: `ensure_parser`, `load_grammar`, `preload_grammar`, `grammar_worker`, + `cache_mutex`; `preload_grammars` is now a serial registry warm-up. The minify + tests dropped their `parse_mu` serialization and preload scaffolding entirely. + +The **later** decision — pool vs per-thread parser reuse for genuine parallel +parsing — is still open, but intentionally so: it only matters once we parse in +parallel, and it now changes only `open_parser`, not the rest of the library. +The original investigation follows. ## How we got here diff --git a/TODOS.md b/TODOS.md index f3e143c..445c5f6 100644 --- a/TODOS.md +++ b/TODOS.md @@ -2,12 +2,15 @@ ## 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). +- [ ] Decide the parallel-parsing strategy for the grammar cache (pool vs + thread-local parsers) — deferred until the parallel usage shape is clear. + - Resolved already: the registry (`grammar()`) is thread-safe and self- + initializing, its allocator is pinned to the heap, and parsers/cursors are now + caller-owned (`open_parser`) instead of cached — so the library holds no + single-thread-use state and the multi-threaded tests pass without locks. + - Remaining choice only matters once we actually parse in parallel: reuse + parsers via a pool or per-thread storage, plugged in behind `open_parser` + without touching call sites. A `TSParser`/`TSQueryCursor` is one-per-thread. - 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. diff --git a/assets.odin b/assets.odin index 3dd2147..49f9f12 100644 --- a/assets.odin +++ b/assets.odin @@ -5,7 +5,20 @@ import "core:log" import "core:os" import "core:strings" +import ts "treesitter" + copy_assets_dir :: proc(vfs: ^VFS, output_dir: string, features: bit_set[Feature]) { + // One CSS parser, reused for every .css asset (single-threaded). Owned here. + css_parser: ts.Parser + if .Minify in features { + if g, ok := ts.grammar("css"); ok { + css_parser = ts.open_parser(g) + } + } + defer if css_parser != nil { + ts.parser_delete(css_parser) + } + for virtual_path, entry in vfs.files { if !strings.has_prefix(virtual_path, "assets/") { continue @@ -24,7 +37,11 @@ copy_assets_dir :: proc(vfs: ^VFS, output_dir: string, features: bit_set[Feature if .Minify in features && strings.has_suffix(rel, ".css") { data, ok := vfs_get(vfs, virtual_path) if ok { - write_file(dest, minify_css(string(data))) + if m, mok := minify_css(css_parser, string(data)); mok { + write_file(dest, m) + } else { + write_file(dest, string(data)) + } } } else if entry.data != nil { if err := os.write_entire_file(dest, entry.data); err != nil { diff --git a/markdown/highlight.odin b/markdown/highlight.odin index e84bafe..0ef5499 100644 --- a/markdown/highlight.odin +++ b/markdown/highlight.odin @@ -125,16 +125,21 @@ unescape_html :: proc(s: string) -> string { } highlight_block :: proc(code: string, lang: string, file_path: string) -> string { - gc := ts.load_grammar(lang) - if gc == nil { + g, ok := ts.grammar(lang) + if !ok { return code } + parser := ts.open_parser(g) + if parser == nil { + return code + } + defer ts.parser_delete(parser) raw_code := unescape_html(code) raw_c := strings.clone_to_cstring(raw_code) defer delete(raw_c) - tree := ts.parser_parse_string(gc.parser, nil, raw_c, u32(len(raw_code))) + tree := ts.parser_parse_string(parser, nil, raw_c, u32(len(raw_code))) if tree == nil { return code } @@ -156,12 +161,13 @@ highlight_block :: proc(code: string, lang: string, file_path: string) -> string } } - cursor := gc.cursor - if cursor == nil { + if g.query == nil { return code } + cursor := ts.query_cursor_new() + defer ts.query_cursor_delete(cursor) - ts.query_cursor_exec(cursor, gc.query, root) + ts.query_cursor_exec(cursor, g.query, root) captures := make([dynamic]Capture, 0, 64, context.temp_allocator) defer delete(captures) @@ -174,7 +180,7 @@ highlight_block :: proc(code: string, lang: string, file_path: string) -> string } cap := match.captures[capture_idx] name_len: u32 - name_c := ts.query_capture_name_for_id(gc.query, cap.index, &name_len) + name_c := ts.query_capture_name_for_id(g.query, cap.index, &name_len) if name_c == nil { continue } diff --git a/markdown/highlight_test.odin b/markdown/highlight_test.odin new file mode 100644 index 0000000..a37c7ad --- /dev/null +++ b/markdown/highlight_test.odin @@ -0,0 +1,83 @@ +#+test +package markdown + +import ts "../treesitter" + +import "core:mem" +import "core:strings" +import "core:testing" + +// highlight_block returns the input string unchanged (borrowed) when it can't +// highlight — no grammar, no query, or zero captures — and a freshly-allocated +// string when it does. Free only the latter; identity (raw_data) tells them +// apart regardless of content. +@(private = "file") +free_highlight :: proc(out, code: string) { + if raw_data(out) != raw_data(code) { + delete(out) + } +} + +// A builtin grammar (css) highlights into hl- spans, preserving the source text. +@(test) +test_highlight_block_css :: proc(t: ^testing.T) { + code := "a{color:red}" + out := highlight_block(code, "css", "test") + defer free_highlight(out, code) + + testing.expect(t, strings.contains(out, "")) + testing.expect(t, strings.contains(out, "color")) + testing.expect(t, strings.contains(out, "red")) +} + +// The other builtin grammar (html) highlights, and the source's own '<'/'>' are +// HTML-escaped in the output (so they can't be confused with the wrapping spans). +@(test) +test_highlight_block_html_escapes_source :: proc(t: ^testing.T) { + code := "

hi

" + out := highlight_block(code, "html", "test") + defer free_highlight(out, code) + + testing.expect(t, strings.contains(out, " shell. +// Run under a scratch arena (as production does) so the intermediate allocations +// highlight_code doesn't individually free are reclaimed in one shot. +@(test) +test_highlight_code_wraps_css_block :: proc(t: ^testing.T) { + arena: mem.Dynamic_Arena + mem.dynamic_arena_init(&arena) + defer mem.dynamic_arena_destroy(&arena) + context.allocator = mem.dynamic_arena_allocator(&arena) + + html := `
a{color:red}
` + out := highlight_code(html, "test") + + testing.expect(t, strings.contains(out, `
`))
+	testing.expect(t, strings.contains(out, "
")) + testing.expect( + t, + strings.contains(out, " string { - gc := ts.ensure_parser("html") - if gc == nil { - return source +// minify_html collapses insignificant whitespace, strips comments, and minifies +// inline

hi

` - result := ts_minify_html(input) + result, ok := minify_html(hp, cp, input) + testing.expect(t, ok) + defer delete(result) // The CSS body is collapsed by minify_css (delimiters lose surrounding // whitespace)... @@ -65,10 +55,15 @@ body { @(test) test_minify_html_empty_style :: proc(t: ^testing.T) { - setup_treesitter() + hp, cp := open_parsers(t) + defer ts.parser_delete(hp) + defer ts.parser_delete(cp) + // An empty has no raw_text child; must not crash. input := `` - result := ts_minify_html(input) + result, ok := minify_html(hp, cp, input) + testing.expect(t, ok) + defer delete(result) testing.expect(t, strings.contains(result, "")) } @@ -76,19 +71,28 @@ test_minify_html_empty_style :: proc(t: ^testing.T) { @(test) test_minify_css_leading_and_comment :: proc(t: ^testing.T) { - setup_treesitter() + cg, cok := ts.grammar("css") + testing.expect(t, cok, "css grammar should load") + cp := ts.open_parser(cg) + defer ts.parser_delete(cp) + // Leading whitespace and a comment between two whitespace runs must not // leave a leading space or a doubled space. - testing.expect_value(t, ts_minify_css("\n/* c */\na {}"), "a{}") - testing.expect_value(t, ts_minify_css(" a {}"), "a{}") - testing.expect_value(t, ts_minify_css("a {}\n/* trailing */"), "a{}") + expect_css(t, cp, "\n/* c */\na {}", "a{}") + expect_css(t, cp, " a {}", "a{}") + expect_css(t, cp, "a {}\n/* trailing */", "a{}") } @(test) test_minify_html_script_preserved :: proc(t: ^testing.T) { - setup_treesitter() + hp, cp := open_parsers(t) + defer ts.parser_delete(hp) + defer ts.parser_delete(cp) + // " - result := ts_minify_html(input) + result, ok := minify_html(hp, cp, input) + testing.expect(t, ok) + defer delete(result) testing.expect(t, strings.contains(result, "const s = \"a b\";")) } diff --git a/render.odin b/render.odin index f883073..8607165 100644 --- a/render.odin +++ b/render.odin @@ -3,6 +3,7 @@ package main import "mustache" import diags "diagnostics" +import ts "treesitter" import "core:encoding/json" import "core:fmt" @@ -205,6 +206,24 @@ render_site :: proc(site: ^Site) { } } + // Parsers for minification, created once and reused across every page + // (render is single-threaded). Owned here; freed when render_site returns. + html_parser, css_parser: ts.Parser + if .Minify in site.features { + if g, gok := ts.grammar("html"); gok { + html_parser = ts.open_parser(g) + } + if g, gok := ts.grammar("css"); gok { + css_parser = ts.open_parser(g) + } + } + defer if html_parser != nil { + ts.parser_delete(html_parser) + } + defer if css_parser != nil { + ts.parser_delete(css_parser) + } + // Render individual content pages (skip all index pages) for page in pages { if page._is_index { @@ -213,7 +232,9 @@ render_site :: proc(site: ^Site) { tpl := get_template(&site.vfs, page.layout, &template_cache) html := render_page_html(page, site, tpl, partials, ctx, &errors) if .Minify in site.features { - html = minify_html(html) + if m, mok := minify_html(html_parser, css_parser, html); mok { + html = m + } } write_page(site.output_dir, page.permalink, html) } @@ -243,7 +264,9 @@ render_site :: proc(site: ^Site) { &errors, ) if .Minify in site.features { - html = minify_html(html) + if m, mok := minify_html(html_parser, css_parser, html); mok { + html = m + } } write_page(site.output_dir, fmt.aprintf("/%s/", section), html) } @@ -253,7 +276,9 @@ render_site :: proc(site: ^Site) { home_tpl := get_template(&site.vfs, "home", &template_cache) home_html := render_home_html(home, site, home_tpl, partials, ctx, &errors) if .Minify in site.features { - home_html = minify_html(home_html) + if m, mok := minify_html(html_parser, css_parser, home_html); mok { + home_html = m + } } write_file(fmt.tprintf("%s/index.html", site.output_dir), home_html) } @@ -445,4 +470,3 @@ write_file :: proc(path: string, html: string) { log.errorf("cannot write %s: %v", path, err) } } - diff --git a/treesitter/treesitter.odin b/treesitter/treesitter.odin index 18d2b11..352b8f4 100644 --- a/treesitter/treesitter.odin +++ b/treesitter/treesitter.odin @@ -7,7 +7,6 @@ import "core:mem" import "core:os" import "core:strings" import "core:sync" -import "core:thread" grammar_dir: string query_dir: string @@ -120,17 +119,15 @@ 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 { +// A Grammar bundles only the immutable, shareable tree-sitter objects for a +// language: `language` (from tree_sitter_x() or a dlopen'd .so) and `query` (the +// compiled highlight query). Both are immutable after construction and safe to +// read from any number of threads. The stateful, single-thread-use objects — +// parser and query cursor — are deliberately NOT stored here; the caller creates +// and owns those (see `open_parser`). See GRAMMAR_CACHE_THREADING.md. +Grammar :: struct { language: Language, - parser: Parser, query: Query, - cursor: Query_Cursor, query_failed: bool, } @@ -138,31 +135,94 @@ 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 { - cache: map[string]^Grammar_Cache, +// registry is the process-lifetime cache of immutable Grammars, keyed by language +// name. It is the library's ONLY shared mutable state; `mu` serializes all access. +// A nil value memoizes "tried and unavailable" so misses don't re-dlopen. Parsers +// live in the caller, so the registry holds nothing single-thread-use. +Grammar_Registry :: struct { + mu: sync.Mutex, + grammars: map[string]^Grammar, 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. +registry: Grammar_Registry + +// init_persistent is optional — grammar() lazily initializes the registry on first +// use — but callers (main) may call it to bind the cache early. The allocator is +// pinned to the OS heap, NOT context.allocator, so the cache survives any transient +// allocator (per-build arena, per-test tracking) that happens to be live. init_persistent :: proc() { - grammar_store.allocator = context.allocator - grammar_store.cache = make(map[string]^Grammar_Cache, grammar_store.allocator) + sync.mutex_lock(®istry.mu) + defer sync.mutex_unlock(®istry.mu) + ensure_registry() +} + +// ensure_registry binds the registry to the heap on first use. Caller must hold mu. +@(private) +ensure_registry :: proc() { + if registry.grammars == nil { + registry.allocator = os.heap_allocator() + registry.grammars = make(map[string]^Grammar, registry.allocator) + } +} + +// grammar returns the immutable Grammar for `lang`, loading it on first use. +// Thread-safe: concurrent callers serialize on `mu`. Returns (nil, false) when the +// grammar cannot be loaded. The returned pointer is immutable — callers may cache +// and read it (its `language`/`query`) without any further locking. +grammar :: proc(lang: string) -> (^Grammar, bool) { + sync.mutex_lock(®istry.mu) + defer sync.mutex_unlock(®istry.mu) + ensure_registry() + if g, seen := registry.grammars[lang]; seen { + return g, g != nil + } + g := build_grammar(lang) + registry.grammars[lang] = g + return g, g != nil +} + +// build_grammar loads a language and compiles its highlight query. Caller must +// hold mu. Returns nil if the language itself cannot be loaded; a grammar whose +// query is absent/failed is still returned (parsing works, highlighting does not). +@(private) +build_grammar :: proc(lang: string) -> ^Grammar { + language, ok := load_language(lang) + if !ok { + return nil + } + g := new(Grammar, registry.allocator) + g.language = language + if query, qok := compile_query(lang, language); qok { + g.query = query + } else { + g.query_failed = true + } + return g +} + +// open_parser creates a fresh parser bound to the grammar's language. The CALLER +// owns the returned parser and must free it with parser_delete. This is the seam +// for parser lifetime: today callers open one and reuse it across a batch of +// parses on a single thread (render opens an html+css pair per render_site; +// assets opens one per copy_assets_dir), which is safe because nothing is shared. +// A future pooling or thread-local strategy can be dropped in here without +// touching any call site. Returns nil on failure. +open_parser :: proc(g: ^Grammar) -> Parser { + if g == nil { + return nil + } + parser := parser_new() + if parser == nil { + log.errorf("treesitter: cannot create parser") + return nil + } + if !parser_set_language(parser, g.language) { + log.errorf("treesitter: ABI mismatch setting parser language") + parser_delete(parser) + return nil + } + return parser } when SPALL { @@ -190,7 +250,7 @@ builtin_language :: proc(lang: string) -> (language: Language, ok: bool) { // load_query returns the highlight query source for a language. Builtin // languages (html/css) are baked into the binary via `#load`; all others are // read from the runtime `query_dir`. `path` is the on-disk location for -// diagnostics ("(builtin)" for embedded queries). Mirrors `ensure_parser`. +// diagnostics ("(builtin)" for embedded queries). Mirrors `load_language`. load_query :: proc(lang: string) -> (src: string, path: string, ok: bool) { switch lang { case "html": @@ -239,50 +299,7 @@ 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 - } - - grammar_store.cache[lang] = nil - - language, ok := load_language(lang) - if !ok { - return nil - } - - parser := parser_new() - if parser == nil { - log.errorf("treesitter: cannot create parser for %s", lang) - return nil - } - if !parser_set_language(parser, language) { - log.errorf("treesitter: ABI mismatch for %s grammar", lang) - parser_delete(parser) - return nil - } - - gc := new(Grammar_Cache, grammar_store.allocator) - gc.language = language - gc.parser = parser - grammar_store.cache[lang] = gc - return gc -} - -compile_query :: proc( - lang: string, - language: Language, -) -> ( - query: Query, - cursor: Query_Cursor, - ok: bool, -) { +compile_query :: proc(lang: string, language: Language) -> (query: Query, ok: bool) { query_src, query_path, qok := load_query(lang) if !qok { return @@ -344,112 +361,20 @@ compile_query :: proc( return } - cursor = query_cursor_new() ok = true return } -load_grammar :: proc(lang: string) -> ^Grammar_Cache { - gc := ensure_parser(lang) - if gc == nil { - return nil - } - if gc.query != nil { - return gc - } - if gc.query_failed { - return nil - } - - query, cursor, ok := compile_query(lang, gc.language) - if !ok { - gc.query_failed = true - return nil - } - - gc.query = query - gc.cursor = cursor - return gc -} - -preload_grammar :: proc(lang: string) -> ^Grammar_Cache { - language, ok := load_language(lang) - if !ok { - return nil - } - - parser := parser_new() - if parser == nil { - log.errorf("treesitter: cannot create parser for %s", lang) - return nil - } - if !parser_set_language(parser, language) { - log.errorf("treesitter: ABI mismatch for %s grammar", lang) - parser_delete(parser) - return nil - } - - gc := new(Grammar_Cache, grammar_store.allocator) - gc.language = language - gc.parser = parser - - query, cursor, qok := compile_query(lang, language) - if !qok { - gc.query_failed = true - return gc - } - - gc.query = query - gc.cursor = cursor - return gc -} - +// preload_grammars warms the registry for the given languages ahead of the render +// loop, so the first page that needs one doesn't pay the load cost inline. Builtins +// are always available lazily and are skipped. Runs on the calling thread; grammar() +// is itself thread-safe if a caller ever wants to fan this out. preload_grammars :: proc(languages: []string) { - if len(languages) == 0 { - return - } - - // Filter out already-loaded languages (watch mode reuse) - to_load := make([dynamic]string, 0, len(languages), context.temp_allocator) for lang in languages { - if cached, ok := grammar_store.cache[lang]; ok && cached != nil { - continue - } if _, bok := builtin_language(lang); bok { continue } - append(&to_load, lang) - } - - if len(to_load) == 0 { - return - } - - threads := make([]^thread.Thread, len(to_load), context.temp_allocator) - for i in 0 ..< len(to_load) { - threads[i] = thread.create_and_start_with_poly_data(to_load[i], grammar_worker) - } - for t in threads { - thread.join(t) - thread.destroy(t) - } -} - -grammar_worker :: proc(lang: string) { - when SPALL { - if _thread_init != nil { - _thread_init() - } - defer if _thread_cleanup != nil { - _thread_cleanup() - } - } - - gc := preload_grammar(lang) - if gc != nil { - sync.mutex_lock(&cache_mutex) - grammar_store.cache[lang] = gc - sync.mutex_unlock(&cache_mutex) + grammar(lang) } }