Build strings through StringBuilder instead of chained concat #44

Merged

agent merged 2 commits from task/1635-stringbuilder into master

2026-10-07 17:41:10 +03:00

agra commented

2026-10-07 17:21:58 +03:00

Owner

Tracker: #1635

Run contract: #1635 comment 12565 (#2607 note revision 24, step 11). Supersedes #3, re-delivered on master c1eb9df; no QueryBuilder.

Strings that grow in a loop or across several appends are written into one StringBuilder.init() (context.allocator) and handed out by finish():

src/store.sx: project and task patch SET clauses (patch_set is a ufcs method on *StringBuilder), the view SET clause, task_filter and query_streams WHERE bodies (and_clause, placeholders), fts_match / fts_quote, order_sql.
src/api.sx: SSE data: lines, event and close frames (sse_data, sse_event write into the caller's builder), a tail round's event batch, an upload's sniffed head; slice_of gives way to the slice operator.
src/logging.sx: the request line; config is one flat variadic concat.
tests/: PR #3's test pass (accumulation loops through StringBuilder, nested concat flattened), plus the same for the loops master added since (file_tags.sx titles, seeded titles, tag pairs; step_files.sx euro name; step9_release.sx curl command).

Every concat left in src/ is a single flat join. Acceptance grep, master c1eb9df → this head: x = concat(x, …) 14 → 0, nested concat(…concat(…)) 12 → 0. The diff adds no FixedWriter, c.memcpy, [*]u8 storage or @reinterpret.

Gate (Server gate + W1, run in order on `6d2291f`)

Line	Exit	Output
S0 toolchain guard (sx 037c4ec9)	0
S1 cold build	0	`compiled: sak`
W0 SPA build (Node 26.3.0)	0
S2 server tests (all but step9)	0	28 tests, each log ends `OK`, no FAIL
S3 release + step9_release	0	three archives (macos-arm64, linux-x86_64, linux-arm64), `STEP9 OK`
W1 `(cd web && npx playwright test)`	0	366 passed, 1 skipped (`@engines` WebKit pinch test, chromium skips it)

S6 is out of this step's gate (it joins from step 47). GOLD: no goldens change.

Output bytes unchanged

This is a construction change; no behaviour differs, so there is no regression that fails on the base. The existing suite pins the outputs it touches (e.g. step_streams_api.sx splits first\nsecond, third\rfourth, fifth\r\nsixth into SSE data: lines; step5_search.sx covers FTS search, labels, attributes and sort). Beyond the suite, one scripted HTTP session was run against ./sak built from master c1eb9df and from this head, same port, fresh database, --log-level debug: 77 exchanges covering project / task / view patches (each field combination, If-Match conflict), task queries (FTS with quotes and blanks, labels, attributes, reference, every sort with and without search), stream event append with CR / LF / CRLF / empty payloads, SSE tails replaying to the close frame with and without a cursor, stream queries by status and opened_after, PNG uploads (full and shorter than the 64-byte sniff head) and a refused non-image, and /api/logs. With timestamps and millisecond durations masked, the response transcripts (43334 bytes; 48048 raw) and the stderr logs (7758 bytes) are byte-identical (cmp exit 0 on both).

Head: 6d2291f03a

Tracker: [#1635](https://sak.swipelab.com/tasks/1635) Run contract: [#1635 comment 12565](https://sak.swipelab.com/tasks/1635) (#2607 note revision 24, step 11). Supersedes https://git.swipelab.com/sx/sak/pulls/3, re-delivered on master c1eb9df; no QueryBuilder. Strings that grow in a loop or across several appends are written into one `StringBuilder.init()` (context.allocator) and handed out by `finish()`: - `src/store.sx`: project and task patch `SET` clauses (`patch_set` is a ufcs method on `*StringBuilder`), the view `SET` clause, `task_filter` and `query_streams` WHERE bodies (`and_clause`, `placeholders`), `fts_match` / `fts_quote`, `order_sql`. - `src/api.sx`: SSE `data:` lines, event and close frames (`sse_data`, `sse_event` write into the caller's builder), a tail round's event batch, an upload's sniffed head; `slice_of` gives way to the slice operator. - `src/logging.sx`: the request line; `config` is one flat variadic `concat`. - `tests/`: PR #3's test pass (accumulation loops through `StringBuilder`, nested `concat` flattened), plus the same for the loops master added since (`file_tags.sx` titles, seeded titles, tag pairs; `step_files.sx` euro name; `step9_release.sx` curl command). Every `concat` left in `src/` is a single flat join. Acceptance grep, master c1eb9df → this head: `x = concat(x, …)` 14 → 0, nested `concat(…concat(…))` 12 → 0. The diff adds no `FixedWriter`, `c.memcpy`, `[*]u8` storage or `@reinterpret`. ## Gate (Server gate + W1, run in order on 6d2291f) | Line | Exit | Output | | --- | --- | --- | | S0 toolchain guard (sx 037c4ec9) | 0 | | | S1 cold build | 0 | `compiled: sak` | | W0 SPA build (Node 26.3.0) | 0 | | | S2 server tests (all but step9) | 0 | 28 tests, each log ends ` OK`, no FAIL | | S3 release + step9_release | 0 | three archives (macos-arm64, linux-x86_64, linux-arm64), `STEP9 OK` | | W1 `(cd web && npx playwright test)` | 0 | 366 passed, 1 skipped (`@engines` WebKit pinch test, chromium skips it) | S6 is out of this step's gate (it joins from step 47). GOLD: no goldens change. ## Output bytes unchanged This is a construction change; no behaviour differs, so there is no regression that fails on the base. The existing suite pins the outputs it touches (e.g. `step_streams_api.sx` splits `first\nsecond`, `third\rfourth`, `fifth\r\nsixth` into SSE `data:` lines; `step5_search.sx` covers FTS search, labels, attributes and sort). Beyond the suite, one scripted HTTP session was run against `./sak` built from master c1eb9df and from this head, same port, fresh database, `--log-level debug`: 77 exchanges covering project / task / view patches (each field combination, If-Match conflict), task queries (FTS with quotes and blanks, labels, attributes, reference, every sort with and without search), stream event append with CR / LF / CRLF / empty payloads, SSE tails replaying to the close frame with and without a cursor, stream queries by status and `opened_after`, PNG uploads (full and shorter than the 64-byte sniff head) and a refused non-image, and `/api/logs`. With timestamps and millisecond durations masked, the response transcripts (43334 bytes; 48048 raw) and the stderr logs (7758 bytes) are byte-identical (`cmp` exit 0 on both). Head: 6d2291f03a6f4312a36eee94ef9600658325b952

agra added 2 commits

2026-10-07 17:21:58 +03:00

Build SQL fragments, SSE frames, upload heads and log lines through StringBuilder 4599c6badf

Loop-grown and multi-append strings in src/store.sx (patch and view SET
clauses, task and stream filters, FTS match and quote, ORDER BY,
placeholders), src/api.sx (SSE data, event and close frames, a tail
round's event batch, an upload's sniffed head) and src/logging.sx (the
request line) are written into one StringBuilder; the helpers are ufcs
methods on *StringBuilder and slice_of gives way to the slice operator.
The remaining concat calls in src/ are single flat joins.

Grow test request text through StringBuilder and flatten nested concat 6d2291f03a

agra referenced this pull request

2026-10-07 17:22:03 +03:00

Build strings through StringBuilder instead of chained concat #3

agent approved these changes

2026-10-07 17:41:05 +03:00

agent left a comment

APPROVED — Cody, reviewing head 6d2291f03a6f4312a36eee94ef9600658325b952 against master c1eb9df6ebfac2225d45c7d43b3833f6de22b504 under run contract #1635 comment 12565 (#2607 note revision 24, step 11; Gate: Server gate + W1, S6 out before step 47).

Gate lines (run by me on the PR head, in order, each from the repo root)

S0 toolchain guard: exit 0 (sx src at 037c4ec9, clean; bin/sx sha256 bcacbe45…).
S1 cold build: exit 0, compiled: sak.
W0 npm ci + npm run build: exit 0.
S2 every server test but step9 (run as the contract's loop, split in two halves for time): exit 0, 28/28 logs end OK, no FAIL (FILE_TAGS, STEP5, STEP_FILES, LOGGING, STREAMS API, CONCURRENCY, … all OK).
S3 release.sh + step9_release: exit 0, three archives (macos-arm64, linux-x86_64, linux-arm64), STEP9 OK.
W1 (cd web && npx playwright test): exit 0, 366 passed, 1 skipped.
GOLD: no file under web/e2e changes in this diff (no goldens).

Acceptance

No growing concat in src/*.sx; a fixed join stays one variadic concat — met. git grep on c1eb9df→6d2291f: nested concat(…concat(…)) in src+tests 21 → 0; self-append x = concat(x, …) → 0. The 16 concat( lines left in src/ are single flat joins (paths, token prefixes, pad2, config's now-flat 3-arg join, " WHERE " + body) or SQL group_concat.
StringBuilder.init() over context.allocator; finish() hands out an owned string with its length; no FixedWriter, c.memcpy, raw [*]u8 storage or @reinterpret — met. StringBuilder.init defaults alloc = context.allocator (std/writer.sx); finish() hands the buffer out as string{ptr,len} and empties the builder, so the caller owns it: request handlers run under the server's per-request arena, tail_round under the tail's scratch arena (push .{ allocator = t.scratch }), so every handed-out buffer returns to the allocator that funded it, on success and raise paths alike (receive_file's raise ApiErr.Refused leaves head to the request arena, as the concat it replaces did). reserve frees each outgrown buffer, so intermediates no longer pile up. An unwritten builder's finish() is an empty string; every such site reads it by .len (fts_match→fts.len > 0, view_set→sets.len == 0, WHERE bodies, empty upload→sniff_type length checks). Diff grep for FixedWriter / c.memcpy / [*]u8 / c.malloc / c.free / @reinterpret: no added line.
Output bytes unchanged; the server tests pass — met. Independent probe: ./sak built from c1eb9df and from this head (same sx), each served over a fresh scratch DB at --log-level debug, same port and DB path, driven by an 84-exchange raw-socket script covering project/task/view PATCH SET clauses (with parent/reference/empty patches and a 409), FTS search with embedded quotes, whitespace-only and multi-term search, every sort with and without search, label/attr/reference filters, /api/streams status/opened_after queries incl. refusals, SSE replay + resume + close frames over CR / LF / CRLF / empty payloads and a CRLF result, uploads crossing and short of SNIFF_LEN, empty, wrong-type and mismatched-type, 404, and /api/logs. With timestamps/ms masked, head runs 1–3 are byte-identical to base run 1 for both responses (45488 B) and stderr (8319 B). Base differs from itself across runs at two timing-dependent points (ms ties in the default -updated order; the chunk boundary between a tail's replay round and its close frame), never in content. S2 above is the server-test half.
Delivered as a PR linking #1635; PR #3 closed pointing to it — met. PR body opens Tracker: [#1635](…/tasks/1635) and cites comment 12565; PR #3 is closed with forge comment 1647 naming this PR.

Review notes

Regression: this is construction-only; nothing fails on the base, and the existing suite pins the touched outputs (step_streams_api's LF/CR/CRLF data: splitting and replay/resume frames, step5_search filters/sorts, step_logging's debug line, streams_across_tasks' status/opened_after query, file_tags/step_files sniffing). No extra regression is needed.
slice_of's to <= from guard is gone; in sse_data start <= i and start <= payload.len always hold, so payload[start .. i] is the same bytes.
Ordering preserved where code moved: order_sql writes FTS_RANK, first, then column/dir, then the t.id tiebreak; patch SET clauses keep updated_at (and revision) before the field list and bind order is unchanged; writeInt renders seq as format("{}") did.
flow.md §1 on changed lines: the one reworded comment (patch_set) states the present contract and its bind-order invariant; no historical text, catalogs, sentinels or guards added. No allocator bypass, no invented @reinterpret.

**APPROVED** — Cody, reviewing head `6d2291f03a6f4312a36eee94ef9600658325b952` against master `c1eb9df6ebfac2225d45c7d43b3833f6de22b504` under run contract [#1635 comment 12565](https://sak.swipelab.com/tasks/1635) (#2607 note revision 24, step 11; Gate: Server gate + W1, S6 out before step 47). ## Gate lines (run by me on the PR head, in order, each from the repo root) - **S0** toolchain guard: exit 0 (sx src at 037c4ec9, clean; bin/sx sha256 bcacbe45…). - **S1** cold build: exit 0, `compiled: sak`. - **W0** `npm ci` + `npm run build`: exit 0. - **S2** every server test but step9 (run as the contract's loop, split in two halves for time): exit 0, 28/28 logs end ` OK`, no FAIL (FILE_TAGS, STEP5, STEP_FILES, LOGGING, STREAMS API, CONCURRENCY, … all OK). - **S3** release.sh + step9_release: exit 0, three archives (macos-arm64, linux-x86_64, linux-arm64), `STEP9 OK`. - **W1** `(cd web && npx playwright test)`: exit 0, 366 passed, 1 skipped. - GOLD: no file under web/e2e changes in this diff (no goldens). ## Acceptance - **No growing concat in src/*.sx; a fixed join stays one variadic concat** — met. `git grep` on c1eb9df→6d2291f: nested `concat(…concat(…))` in src+tests 21 → 0; self-append `x = concat(x, …)` → 0. The 16 `concat(` lines left in src/ are single flat joins (paths, token prefixes, `pad2`, `config`'s now-flat 3-arg join, `" WHERE " + body`) or SQL `group_concat`. - **StringBuilder.init() over context.allocator; finish() hands out an owned string with its length; no FixedWriter, c.memcpy, raw [*]u8 storage or @reinterpret** — met. `StringBuilder.init` defaults `alloc = context.allocator` (std/writer.sx); `finish()` hands the buffer out as `string{ptr,len}` and empties the builder, so the caller owns it: request handlers run under the server's per-request arena, `tail_round` under the tail's `scratch` arena (`push .{ allocator = t.scratch }`), so every handed-out buffer returns to the allocator that funded it, on success and raise paths alike (`receive_file`'s `raise ApiErr.Refused` leaves `head` to the request arena, as the concat it replaces did). `reserve` frees each outgrown buffer, so intermediates no longer pile up. An unwritten builder's `finish()` is an empty string; every such site reads it by `.len` (`fts_match`→`fts.len > 0`, `view_set`→`sets.len == 0`, WHERE bodies, empty upload→`sniff_type` length checks). Diff grep for FixedWriter / c.memcpy / [*]u8 / c.malloc / c.free / @reinterpret: no added line. - **Output bytes unchanged; the server tests pass** — met. Independent probe: `./sak` built from c1eb9df and from this head (same sx), each served over a fresh scratch DB at `--log-level debug`, same port and DB path, driven by an 84-exchange raw-socket script covering project/task/view PATCH SET clauses (with parent/reference/empty patches and a 409), FTS search with embedded quotes, whitespace-only and multi-term search, every sort with and without search, label/attr/reference filters, `/api/streams` status/opened_after queries incl. refusals, SSE replay + resume + close frames over CR / LF / CRLF / empty payloads and a CRLF result, uploads crossing and short of SNIFF_LEN, empty, wrong-type and mismatched-type, 404, and `/api/logs`. With timestamps/ms masked, head runs 1–3 are byte-identical to base run 1 for both responses (45488 B) and stderr (8319 B). Base differs from *itself* across runs at two timing-dependent points (ms ties in the default `-updated` order; the chunk boundary between a tail's replay round and its close frame), never in content. S2 above is the server-test half. - **Delivered as a PR linking #1635; PR #3 closed pointing to it** — met. PR body opens `Tracker: [#1635](…/tasks/1635)` and cites comment 12565; PR #3 is closed with forge comment 1647 naming this PR. ## Review notes - Regression: this is construction-only; nothing fails on the base, and the existing suite pins the touched outputs (step_streams_api's LF/CR/CRLF `data:` splitting and replay/resume frames, step5_search filters/sorts, step_logging's debug line, streams_across_tasks' status/opened_after query, file_tags/step_files sniffing). No extra regression is needed. - `slice_of`'s `to <= from` guard is gone; in `sse_data` `start <= i` and `start <= payload.len` always hold, so `payload[start .. i]` is the same bytes. - Ordering preserved where code moved: `order_sql` writes `FTS_RANK, ` first, then column/dir, then the `t.id` tiebreak; patch SET clauses keep `updated_at` (and `revision`) before the field list and bind order is unchanged; `writeInt` renders `seq` as `format("{}")` did. - flow.md §1 on changed lines: the one reworded comment (`patch_set`) states the present contract and its bind-order invariant; no historical text, catalogs, sentinels or guards added. No allocator bypass, no invented @reinterpret.

agent merged commit bc2b3118d0 into master

2026-10-07 17:41:10 +03:00

agent referenced this pull request from a commit

2026-10-07 17:41:11 +03:00

Merge pull request 'Build strings through StringBuilder instead of chained concat' (#44) from task/1635-stringbuilder into master