v0.19: Replace SXML with typed XML and HTML modules #1515
No reviewers
Labels
No labels
UX
active development
backlog
blocker
bootstrap
bounty
bug
dependencies
discussion
documentation
duplicate
enhancement
flaky test
help wanted
invalid
javascript
question
release
tendentious
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
mighty-gerbils/gerbil!1515
Loading…
Reference in a new issue
No description provided.
Delete branch "jay/gerbil:v0.19-sxml-to-typed-xml"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Replaces SXML in the markup stack with typed XML and HTML modules, as discussed in #1509. SXML stays available, deprecated.
Changes
:std/text/markup/xml: typed XML nodes (XMLElement,XMLText, … under anXMLbase),read-xmlandwrite-xml. Input can be a string, bytes, a port or aReader. Bytes are decoded as UTF-8 (SSAX decoded them as Latin-1).:std/text/markup/html: an HTML parser that follows the WHATWG spec (tokenizer, all 2231 named character references, tree construction, fragment parsing) and an HTML writer. The parser passes the html5lib tokenizer tests and the WPT tree construction tests. Documents nested deeper than 512 elements are refused withHTMLParseError, as Go'sx/net/htmldoes.:std/encoding/utf8: UTF-8 encoding, decoding and validation with exact error offsets, used by the markup modules.djot->sxmlis replaced bydjot->html-fragment.djot->htmloutput is byte-identical on the 302 Djot conformance fixtures.tal:attributesworks.:std/text/markup/sxml.Nodes are immutable values. Applying a node type, e.g.
(XMLCData …), checks its arguments, including list members; themake-xml-*andmake-html-*names are the same constructors, andmake-html-element,make-html-attributeandmake-html-foreign-attributeadd the HTML rules. The writers trust slot types and check what each output language forbids.Breaking changes
:std/text/markup/xmland:std/text/markup/htmlexport the typed APIs. Code using SXML should import:std/text/markup/sxml.djot->sxmlis removed. Djot writes attribute names in lower case, and U+0000 in text and attribute values as U+FFFD.tal:attributes) and templates the HTML parser would read differently.html-escape, novar.repeat), several statements follow the Zope rules, static markup is re-serialized by the HTML writer, andtal:writeis a procedure; see tal.md, "Differences from the SXML TAL".:std/text/markup/sxml/tal/…,sxml/html/tal) is removed;:std/text/markup/talreplaces it.HTMLParseError, and HTML attribute names with ASCII upper case are rejected by the constructors andwrite-html.Notes for review
string-same?, the name tables, the pairwise duplicate check) cites themarkup-benchmark.ssmode, size and iterations that reproduce its measurement.:std/encoding/utf8does not shadow the corestring->utf8andutf8->string. Itsutf8-decoderaises on invalid input instead of writing U+FFFD as the old:std/text/utf8did.src/std/text/markup/html-conformance.ssruns the pinned upstream html5lib and WPT tests from checkouts you supply, andsrc/std/text/markup/markup-benchmark.ssreproduces the performance and scaling claims. Neither is built or run bybuild.sh; their headers give the commands, asdjot-benchmark.ssandhttp-benchmark.ssdo.write-htmlwrites a quirks-forcing DOCTYPE as a well-formed one, so the document may read back in no-quirks mode. This is documented.std/io/biogainsbio-output-reserve!, which makes room for n bytes in an output buffer; when the sink is a stdioBufferedWriter, the writers write straight into its buffer. A port receives nothing when a writer raises.Benchmarks
Speedup over the SXML implementation (old time ÷ new time) for each operation over a corpus of generated and real files. Gerbil compiled with
gxc -O -exeon an AMD Ryzen 9 7940HS, one pinned core, median of seven timed batches. Every input is faster.The largest XML write factors come from the old writer's quadratic behaviour on deep and element-dense trees. A further 23 inputs crash the old implementation (numeric character references ≥ 128, a TAL template using
tal:attributes) and are excluded.Tests
html-conformance.ssagainst html5lib-tests c777c40 and WPT 260bb4c1d9: tokenizer 7045 passed, tree construction 1963 passed, write-html round trip 1950 passed, 0 failed; the 29 skipped cases are listed by id with their reasons in the runner.std/...: 158 modules, 0 failures.:std/encoding/utf8were also fuzzed, and every per-element or per-token loop has a scaling mode inmarkup-benchmark.ss.impressive speedups!
this is massive, but very much needed work. first pass, need to digest it.
can you post the djot benchmark numbers compared to C and js?
astra is reviewing and is quite critical of opus's code already :)
Request changes on
49b02af0. I found reproducible correctness defects and substantial avoidable machinery. The strongest maintainability concern is duplicated infrastructure and semantic rules that have already drifted apart.I reviewed the branch against its merge base with
origin/v0.19-staging.[P2] XML attribute checking is quadratic on untrusted input.
xml/scanner.ss:792 compares each plain attribute against all preceding attributes. The namespace-resolution path has another pairwise comparison at line 739.
Parsing one element with 1,000, 2,000, and 4,000 distinct attributes took approximately 0.37, 1.27, and 5.06 seconds in interpreted execution. These are scaling measurements, not compiled-performance benchmarks, but the quadratic algorithm is explicit.
Use a hash set keyed by expanded attribute name. The HTML emitter already uses a small-list/hash-table threshold for duplicate detection; XML should reuse that approach.
[P2] The HTML parser silently changes tree-building semantics beyond 512 open elements.
html/builder.ss:512 limits scope searches and reports "not in scope" when the target lies outside its window.
Reproduction:
<div>, followed by 513<span>tags, followed by</div><p>after</p>. The resulting fragment has only the outerdivat the top level; the paragraph remains buried inside it. Processing</div>should close the intervening spans and allow the paragraph to become a sibling.A resource limit should produce an explicit failure or a deliberately specified recovery policy. Returning a false scope result silently corrupts parsing.
[P2] TAL dynamic content loses a leading newline in
textarea.tal/compiler.ss:308 chooses a value writer without handling the newline-sensitive element class.
With
<textarea tal:content="x"></textarea>andx = "\nx", TAL emits<textarea>\nx</textarea>. Parsing that output produces"x": HTML consumes the first newline.The HTML emitter and TAL's static-children path already handle this case. Dynamic content needs the same rule, preferably through a shared procedure rather than another independent special case.
[P2] TAL incorrectly turns foreign-content CDATA into a comment.
tal/reader.ss:231 derives CDATA permission from the child insertion context. That differs from the current element's namespace at HTML integration points.
For:
TAL emits:
The HTML reader correctly returns text, producing
<svg><foreignObject>hi</foreignObject></svg>when serialized. This is a concrete consequence of maintaining separate versions of the foreign-content rules.[P2] TAL's repeat-method optimization breaks valid Scheme identifiers.
tal/compiler.ss:549 searches raw attribute strings to decide which bindings to generate, using a handwritten approximation of Scheme identifier syntax.
This valid template fails:
Both iterations render an unbound-variable error for
repeat/i.number. Pipe quoting denotes the same identifier, but the string search misses it.Generate the small, fixed set of bindings and let the compiler optimize them, or inspect actual Scheme syntax. A partial Scheme lexer is excessive machinery for this optimization.
[P2] Script-tokenizer shortcuts bypass carriage-return normalization.
html/scanner.ss:973 jumps past the separator following matched script names without processing that separator.
Parsing
"<script><!--<script\r></script>--></script>"leaves the literal\rin the script text instead of normalizing it to\n. The corresponding</script\r>transition also reproduces the problem.Process the separator through the common normalization logic, or correctly propagate the need to normalize the output span.
[P2] XML declarations are accepted without enforcing their meaning.
xml/scanner.ss:1237 checks only that the version begins with
"1.", acceptingversion='1.foo'.Separately, line 1272 treats ASCII declarations as unrestricted UTF-8. UTF-8 bytes for
<?xml version='1.0' encoding='US-ASCII'?><a>é</a>are accepted and produceé.Validate the version grammar and supported versions, and enforce ASCII for byte input declaring ASCII.
Structural cleanup and reuse of existing facilities
Remove the elaborate support for prohibited mutation.
xml/types.ss:5 says callers should not mutate nodes or their lists after construction. Nevertheless, xml/emitter.ss:8 explicitly caters for mutation by writer callbacks or another thread.
This produces an
untrustedmacro that erases type information, repeated slot checks, attribute snapshots, string-length rechecks, and defensive copies. html/emitter.ss:463 copies individual text strings before the surrounding code may concatenate them again.Settle the ownership contract and trust it internally. Keep checked public constructors, external-input validation, and serialization-validity checks. The machinery for concurrent mutation is a separate feature with considerable complexity and allocation cost; the stated API contract gives it no justification.
Use existing I/O facilities instead of building a private buffering subsystem.
xml/source.ss:14 implements read-all buffering already supplied by
:std/io'sread-all-from-readerand:std/misc/ports'sread-all-as-string/read-all-as-u8vector.xml/emitter.ss:37 combines generic byte buffering with XML namespace state. HTML and TAL consequently depend on an XML emitter as their generic output buffer.
Separate namespace bookkeeping from output. Start with the buffered/memory writers and adapters already available through
:std/io, including with-buffered-writer. A scanner sentinel needs only a thin adaptation. Any custom fast path should have reproducible benchmark evidence showing why the existing abstraction is inadequate.Consolidate HTML semantic rules.
Namespace/context classification and integration-point handling are repeated across the builder, emitter, and TAL reader. Script-state logic also exists in both scanning and serialization.
Shared metadata and transition helpers would reduce the maintenance burden. The CDATA and leading-newline defects above demonstrate the practical cost of the current duplication. The different consumers can retain their own traversal algorithms while sharing these rules.
Replace trivial reinventions and generate mechanical repetition.
html/node.ss'sstring-tablelist->hash-table-stringhtml/emitter.ss'scontains?string-contains, converting its result to a boolean if necessaryxml/node.ss'sstring-same?string=?, unless measurements justify specializationascii-ci=?and emitterfolded-equal?tag-*declarationsThe Djot declarations begin at djot/html.ss:19. A macro preserves their allocation behavior without the repetitive definitions. Much of Djot's renderer-dispatch boilerplate predates this PR, so I would not attribute that existing repetition to this change.
Reduce duplicated UTF-8 machinery.
Input conversion, node emission, scanning, and output decoding each contain handwritten UTF-8 work. In particular, xml/emitter.ss:90 implements another decoder for output already known to be valid UTF-8.
Use existing conversions for those cases. XML-specific character validation still belongs in the XML parser. If specialized encoding loops are measurably necessary, share their implementation or generation rather than maintaining several copies.
The generated entity/atom tables are reasonable, and
define-string-emitteralready demonstrates useful macro factoring. The cleanup should concentrate on unnecessary defensive machinery, duplicated library facilities, and duplicated semantic rules.Validation: ten XML/HTML test modules and the S3 response tests passed using the installed runtime with the PR's sources. TAL's interpreted cases passed; its compiled hygiene case could not complete because the source-only setup lacked class optimizer metadata. Djot tests could not load because the installed dependencies lacked
char-set:punctuation. I did not perform a full build or modify repository files.review by astra, quite critical for the slop factor.
@vyzo addressing your feedback in separate messages so it's easy to reply to indivdually
html->sxml<div><span><span>a</div><p>after</p>comes out as(div (span (span a)) (span (p after))). The review's example is broken in the old library at any depth.x/net/htmlsetMaxDepth.html-parsingWhat do you think we should do? I would lean towards explicitly refuse like Go does, seems the simplest.
Yeah it's quadratic but a hash set doesn't fix it. Gerbil's string hash is FNV-1a and the table seed doesn't change which names collide, so someone can pick attribute names that all land in the same bucket. I had Opus try it and 16k colliding names take 567 ms in a hash set, and the HTML emitter's existing check takes over a second. Sorting and comparing neighbours can't be gamed that way, so I'll do that above 16 attributes and switch the HTML emitter to it too if that sounds good.
The grammar, yes but if "supported versions" means rejecting 1.1, that's wrong: XML 1.0 §2.8 says a 1.0 processor has to accept any 1.x document and treat it as 1.0. So
1.foogets rejected and1.1doesn't.Opus measured this and it's much slower. Our writer escapes and UTF-8-encodes in one pass straight into a byte buffer.
BufferedWriterhas no public way to write into its buffer directly (correct?), so the same work through its methods is 2.6× slower withwrite-string-utf8and 8.6× slower withwrite-char-utf8.So I'll keep a small byte buffer, but move it out of the XML emitter so HTML and TAL stop depending on it if that sounds good.
If you'd take a public way to reserve space in a BufferedWriter's buffer, we can do that in a separate PR?
These two haven't drifted. I had Opus fuzz them against each other with 60k random scripts and they agree on all of them. The CR bug in #6 is a shortcut in the scanner, not a disagreement between the two. The scanner works on bytes in the hot loop and the writer works on strings, so merging them would slow parsing down. I'd rather keep both and add the fuzz test to the suite so any drift gets caught.
They do,
string=?is n-ary, and per element it measured about 1300 instructions against 500 forstring-same?. I'll add a comment saying so.Agree they should go, but we probably don't need a macro? The HTML parser already keeps one shared name object per known tag, so Djot can use those and all 36 definitions disappear.
Tried that and they're slower.
utf8->stringmakesxml->string10–19% slower end to end, andstring->utf8slows string input by 16–44%, because both make two passes over the data. So we can go with your alternative of one shared decoder and validator. Encoding was already a single macro.I think the Djot test failure is your installed runtime:
char-set:punctuationcame in with #1509.The TAL hygiene failure is my test's fault. It assumed compiled TAL metadata was on the loadpath, and it'll skip when it isn't.
i'll do a second pass for matters more of idiom.
Second pass: type hierarchy, signatures, and idiomatic field access
Request changes on
49b02af0585ee3884039e7b46477ce70cf401ce2. TheXMLroot is feasible, and this cleanup should happen before merging. This review supplements the first review.The implementation has typed structs, but their naming and use are inconsistent. The parser frequently uses typed dotted access; TAL, S3, and parts of the HTML API revert to nested accessor calls. The emitters deliberately discard type information and reconstruct it through manual checks.
Please make the following changes across the implementation and its consumers.
Introduce an
XMLbase struct and rename the concrete XML types.The declarations in xml/node.ss are nine unrelated, lowercase struct types. Use an empty, extensible
XMLbase and keep concrete types final:Apply this mapping consistently:
xml-nameXMLNamexml-attributeXMLAttributexml-elementXMLElementxml-textXMLTextxml-cdataXMLCDataxml-commentXMLCommentxml-piXMLPIxml-doctypeXMLDoctypexml-documentXMLDocumentXMLis a nominal model root.XML?must not replace the predicate for valid element children: names, attributes, documents, and doctypes are not legal children merely because they share that root. Preserve the existing context-specific restrictions.Rename the HTML additions to
HTMLRaw,HTMLDoctype, andHTMLDocument, retaining their distinct representations and their reuse ofXMLElement,XMLAttribute, etc. In particular, HTML and XML doctypes have different validity rules.Ordinary procedures such as
read-xml,xml-name=?, and checkedmake-xml-*factories can retain conventional lowercase names. Update generated predicates and accessor names consistently. Preserve the checked-construction boundary; a naming migration must not replace validated construction with unchecked allocation.Convert consumers to typed dotted access, including nested fields.
The clearest cleanup targets are:
doc.root,name.local,name.uri,el.children, andel.name.local.attr.name.localandattr.value.For example, S3's name predicate should become:
For an attribute list already established by the parser or constructor:
For a heterogeneous value, retain the actual dispatch and annotate the selected branch:
Use parameter signatures when the whole procedure operates on one concrete type. Use
usingfor local narrowing or values retrieved from heterogeneous collections. Avoid repeated casts around individual field reads.Preserve type information instead of erasing and rechecking it.
xml/node.ss:237 defines:
This defeats the typed model. For example, XML attribute emission reads an attribute through
using, erases the types of its fields, checks them again, then repeats this for the name's fields.Resolve the ownership issue identified in the first review and remove this machinery under the documented no-mutation contract. Typed
attr.name,attr.value, andattr.name.localshould remain typed.Keep validation that types do not establish: namespace constraints, valid characters, duplicate attributes, legal child kinds, and serialization restrictions. Also retain element validation at public list-taking boundaries:
:listestablishes a proper list, not the types of its members.Replace unnecessarily broad signatures with the actual types.
Several known types are currently spelled
:t:new-elementnameargument isXMLName.freeze-elementXMLElement.token-attribute(Maybe :string); propagate that to encoding parameters.attribute-tableHashTable.positionsfields areHashTable.emit-doctype!XMLDoctype.Likewise, internal child-list procedures should receive
:listonce the boundary establishes that contract. Use:? Typefor nullable parameters and fields, and precise return signatures.Some values genuinely remain heterogeneous: reader sources, HTML writer inputs that also accept fragments, scanner result registers, and user Scheme expressions. Keep those broad where necessary and narrow at dispatch points.
Use forward types for the HTML builder's recursive links.
html/builder.ss:166 justifies six
:tfields with “a slot type cannot name its own struct.” Gerbil provides forward type aliases.Rename
hnodetoHTMLBuilderNodeand use:Its
parent,prev,next,first,last, andcontentsfields can then use:? @HTMLBuilderNode;namecan use:? XMLName. After a nonfalse check, access the typed value directly.This removes a real source of lost type information and repeated
usingblocks. The builder node remains an internal mutable construction representation, separate from the public XML records.Apply the convention to newly introduced implementation types and macros.
Use descriptive CamelCase names such as
XMLScanner,XMLEmitter,HTMLTokenizer,HTMLBuilder,HTMLEmitter,TALTemplate,TALCompiler, andTALRender. Keep these implementation-state types outside theXMLmodel hierarchy.Include field writes in the cleanup. For example, TAL's emitter reset should use typed bindings and
(set! em.pos 0). TAL runtime macros should establish a typed render binding and use fields such asr.out.pos.Preserve macro hygiene and single evaluation when adding bindings. Keep semantic operations—such as checked tokenizer operations—as procedures; distinguish those from generated field accessors.
Complete the public API migration and verify it across module boundaries.
Update both public facades, type modules, signatures, imports, predicates, tests, and the XML/HTML/TAL/Djot documentation together. Public examples should demonstrate typed dotted access. Export the type information consumers need without exposing implementation structs or unchecked construction helpers through the facade.
Acceptance checks should cover:
Follow src/std/AGENTS.md for aligned, one-argument-per-line signatures and typed callbacks. Avoid blanket unchecked annotations where the input type has not been established.
Validation for this pass: static source review plus small interpreted checks confirming struct inheritance, nested dotted access, and nullable recursive fields through
deftype. No repository files were changed, and no build or regression suite was run.if we are getting a new implementation of string->utf8 and utf8->string again, then abstract them into a separate module. we had that before, but went back to gambit forms. either in
:std/string/utf8or (better i think) in:std/encoding/utf8.@jay wrote in #1515 (comment):
yeah, let's refuse and raise an error.
@jay wrote in #1515 (comment):
ok, let's leave that for follow up work.
@jay wrote in #1515 (comment):
as i said above, let's make std/encoding/utf8 for the fast variants. we had them before gambit had native procedures.
@jay wrote in #1515 (comment):
yeah, makes sense.
@vyzo feedback addressed!
ok, i am doing another astra pass and if she's happy i'll do a manual pass.
Third review pass: coding conventions, abstraction, and remaining complexity
I would still request changes on
97080fa9, although this revision substantially addresses the previous reviews. TheXMLhierarchy, CamelCase names, typed dotted access, recursive types, and removal of type-erasing defensive checks are largely done.The remaining concerns are more concentrated: one demonstrated performance problem, an unnecessarily complicated rule-sharing mechanism, and some unresolved API and maintenance issues.
[P2] TAL now does quadratic work on ordinary nested elements.
In tal/reader.ss:209, every HTML start tag calls
open-p-in-button-scope?before checking whether that tag could close a paragraph. The scope check walks the open-element stack.Consequently, nested
<span>elements repeatedly scan all their ancestors—even when the template contains no paragraph at all. I reproduced the following timings forread-tal-template:These are interpreted timings, not compiled benchmarks, but the quadratic traversal is explicit in the implementation.
Requested change: perform a cheap start-tag classification before searching ancestors. Where repeated scope queries remain necessary, maintain the relevant scope information in the open-element state. Add a nested non-paragraph regression case and a reproducible scaling check.
TAL invokes the HTML parser to implement a classification predicate. This is the clearest remaining abstraction problem.
html-start-tag-closes-p? constructs
"<p><" + name + ">", creates scanner input and a tokenizer, builds a fragment, and examines its resulting sibling list.That is an excessive amount of machinery for deciding whether a start tag closes a paragraph. It also makes the decision indirectly depend on a synthetic fragment's parsing context.
Requested change: extract the actual rule into
html/rules.ssand have both the builder and TAL use it. Represent context-dependent conditions explicitly. Avoid either a second copied tag list or memoization around the miniature parse; neither fixes the underlying abstraction.This belongs with the previous finding: simply reversing the current predicates would put a miniature HTML parse on every start tag.
The ordinary public type constructors still bypass the advertised construction boundary.
xml/types.ss:9–14 now explicitly documents that applying an exported type identifier invokes unchecked allocation. Thus
make-xml-cdatavalidates its input, while the natural CamelCase spelling(XMLCData "]]>")bypasses that validation.This is an API design objection, not an undocumented contract violation: the new comment acknowledges the behavior. Nevertheless, documenting undefined behavior for ordinary constructor syntax leaves the checked-construction boundary weaker than the previous review requested.
Requested change: make normal public construction perform validation, using the existing constructor/initializer mechanisms, and retain a clearly internal allocation path for parsers that already establish the invariants. Preserve type exports for signatures and dotted access. Do not compensate by restoring repeated validation throughout the writers.
There are still private replacements for existing runtime functionality, with insufficient reproducible justification.
Two specific examples deserve another pass:
string=?; the runtime already uses the binary##string=?primitive in its string-table implementation.Requested change: compare these implementations against the appropriate existing facilities under compiled execution. Prefer the existing implementation unless the specialized version demonstrates a meaningful benefit. If retained, include the benchmark and explain the workload that warrants the maintenance cost.
The same standard should apply to the private
&string-empty?macro. A performance comment alone does not let subsequent maintainers reproduce or reassess the decision.The strongest conformance and optimization evidence remains outside the repository.
The PR reports substantial external HTML conformance testing, but the runner is not included. The checked-in focused tests are useful; they do not reproduce those corpus-wide claims. Several optimization comments likewise give precise measurements without the corresponding driver.
Requested change: include a runner accepting locally supplied upstream fixtures, document pinned revisions and invocation commands, and make expected skips explicit and checked. Include the small benchmark drivers supporting the retained custom machinery. The existing Djot conformance setup provides a useful precedent.
A smaller, nonblocking macro opportunity remains in html/tokenizer.ss:133–167: seven field wrappers repeat the same typed-argument, kind-check, and dotted-access pattern. A small declaration macro could generate those procedures while preserving their signatures and error attribution. Keep accessors that perform actual conversion explicit.
Validation: all 11 selected UTF-8, XML, HTML, and S3 test modules passed using the updated sources with the installed interpreter. Targeted TAL checks also passed for dynamic textarea newlines, quoted repeat-method references, and foreign-content CDATA. I did not run a full build or the external conformance corpus. No repository files were changed.
astra still has complaints
@vyzo latest feedback addressed!
Fourth review pass: approval with minor follow-ups
Reviewed
3b85ab6d. The previous blocking findings are addressed; this pass found no new merge-blocking defect.The implementation now resolves the earlier concerns directly:
##string=?. I haven't independently verified those compiled performance figures, but the earlier lack of reproducible workloads is resolved.Two minor follow-ups remain:
[P3] The conformance runner's exception-reporting path throws another exception.
In html-conformance.ss:303–308, a tokenizer exception becomes a list containing its message string. That value then goes through
normalize, which expects each entry to be a token list and callscaron it.If tokenization raises, the runner therefore aborts during normalization instead of recording the original failure with its case ID and continuing.
Fix: record exceptions separately from token results, or wrap execution and comparison together and append a failure from the handler. Preserve the original exception and case ID.
Small convention cleanup.
markup-benchmark.ss:27 still puts both parameters on one line, contrary to the signature convention.
git diff --checkalso reports an extra blank line at EOF inhtml/reader-test.ss.These are minor; I don't see justification for another broad abstraction or naming rewrite.
Validation was substantially stronger this time:
The original 800-level TAL example fell from approximately 1.79 seconds to 0.072 seconds in interpreted execution. The source inspection confirms that the quadratic scope traversal was removed.
Testing used the updated sources with the installed interpreter. Compiled TAL hygiene was skipped because optimizer metadata was unavailable. Djot tests could not load because the older installed runtime lacks
char-set:punctuation; this is an environment limitation, not a demonstrated PR regression.No source files were changed.
Fourth review pass: approval with minor follow-ups
Reviewed
3b85ab6d. The previous blocking findings are addressed; this pass found no new merge-blocking defect.The implementation now resolves the earlier concerns directly:
##string=?. I haven't independently verified those compiled performance figures, but the earlier lack of reproducible workloads is resolved.Two minor follow-ups remain:
[P3] The conformance runner's exception-reporting path throws another exception.
In html-conformance.ss:303–308, a tokenizer exception becomes a list containing its message string. That value then goes through
normalize, which expects each entry to be a token list and callscaron it.If tokenization raises, the runner therefore aborts during normalization instead of recording the original failure with its case ID and continuing.
Fix: record exceptions separately from token results, or wrap execution and comparison together and append a failure from the handler. Preserve the original exception and case ID.
Small convention cleanup.
markup-benchmark.ss:27 still puts both parameters on one line, contrary to the signature convention.
git diff --checkalso reports an extra blank line at EOF inhtml/reader-test.ss.These are minor; I don't see justification for another broad abstraction or naming rewrite.
Validation was substantially stronger this time:
The original 800-level TAL example fell from approximately 1.79 seconds to 0.072 seconds in interpreted execution. The source inspection confirms that the quadratic scope traversal was removed.
Testing used the updated sources with the installed interpreter. Compiled TAL hygiene was skipped because optimizer metadata was unavailable. Djot tests could not load because the older installed runtime lacks
char-set:punctuation; this is an environment limitation, not a demonstrated PR regression.No source files were changed.
astra is satisfied; let's address the minor follow ups and i'll do a manual pass tomorrow.
@jay can you post the latest djot benchmarks? I want to see our progress.
@vyzo I have a local branch that speeds up the Djot parsing by about 2x, at some point I will get a PR out for that.
@vyzo also if it's faster for you to make any changes in the name of style / idiomaticity yourself, you are more than welcome to. otherwise I will address whatever additional feedback you have
yeah, it's fine. i'll just do a pass myself to see if anything stands out.
now i have some concerns.
@ -0,0 +119,4 @@(start :- :fixnum)(end :- :fixnum)(tail :- :fixnum))=> :u8vectorshouldn't we just shrink the u8vector to the right size instead of filling the end?
@ -0,0 +57,4 @@(defconst ignored-column-group 2);;; An emitter at the top level, writing to out.(def (make-html-emitter (out :- ByteOutput)what is a ByteOutput? can't we just use a BufferedWriter?
@ -0,0 +21,4 @@;;; BufferedWriter), a byte port, or a character port (as characters).;;; Raises ContractViolation for any tree the HTML parser would not read;;; back as written; html.md gives the output and every rejected case.(def (write-html (node : :t)the types are too wide here.
@ -0,0 +24,4 @@(def (write-html (node : :t)(sink : :t := (current-output-port))scripting?: (scripting? : :boolean := #t)) => :void(check-html-node node write-html)this could be typed at the procedure signature instead of a check here.
@ -0,0 +36,4 @@emit-ascii!)(declare (not safe))(defstruct ByteOutputthis effectively duplicates the memory output buffer.
i have concerns, i'll ask astra about another pass more targeted.
Follow-up review: consolidate ByteOutput into stdio and tighten type contracts
Request changes on
91e655af. This supersedes my earlier approval. The separate buffering implementation needs to be consolidated into stdio before merge. The type-contract cleanup below is also part of the requested changes.I checked the update from
3b85ab6dto91e655af: it addresses the earlier tokenizer-exception reporting and formatting findings. Those findings are resolved by source inspection. The output implementation and the production signatures discussed below are unchanged.1. Consolidate ByteOutput into stdio; do not maintain a second buffering subsystem.
markup/output.ss:39 defines a private buffer, cursor and optional Writer sink, then implements allocation, growth, reservation, flushing and extraction. These are stdio responsibilities. The missing public reservation operation is a reason to extend BufferedWriter interface and its stdio buffer APIs, not a reason to maintain parallel buffering inside markup or defer consolidation to another PR.
Required implementation direction:
Validate reservation larger than current capacity, partial commit, growth preserving prior bytes, UTF-8 split boundaries and sink failures, then rerun the relevant markup/stdio regressions, conformance checks and benchmarks. This is a blocking architectural requirement, not a cosmetic rename.
2. Express the public writer domain in contracts instead of accepting
:tand checking it in the body.xml/writer.ss:12 and html/writer.ss:15 declare
node : :t, then immediately callcheck-xml-node/check-html-node. Centralize the accepted domain in reusable predicate contracts and use those in all three public writer entry points for each format.For XML, the intended shape is:
Do not simply replace this with
XML?: the base class also includes names, attributes and other objects that are not standalone writable nodes. For HTML, define the corresponding accepted domain for a document, a node, or a proper list of permitted fragment nodes. Keep semantic serialization checks in the emitter; a signature cannot establish namespace consistency or round-trip validity.3. Put list element types in constructor contracts; remove the resulting duplicate member checks.
Concrete sites:
Use the existing
list-of?contract machinery where it expresses the domain, for example:Use the appropriate predicate for each position; document prolog/epilog do not accept arbitrary XML nodes. These contracts establish proper lists and member types. After strengthening the contracts, remove manual checks that merely repeat those facts, and use typed callbacks/dotted access for the validated members. Retain checks for duplicate attributes, namespace agreement, doctype ordering and other relationships between values.
The current
:listannotations do NOT establish member types, so deleting the body checks without strengthening the contracts would be wrong.4. Narrow internal parameters and return types where call sites establish a smaller domain.
code!receives constructed nonempty Scheme forms at its call sites. Changeform :- :ttoform :- :pair; arbitrary user expressions can still occur inside those forms.optional-end-closers-ofreturns a list or the sentinel0, forcing=> :t. Use#ffor absence and=> (Maybe :list); update callers accordingly.newline-text?andparagraph?inspect internally rendered XML/HTML nodes. Give their parameter the establishedXMLtype rather than:t; retain the concrete subtype tests that distinguish text from elements.render-referenceaccepts anydjot-container, although the actual variants are links and images. Let typed link/image entry points extract the shared reference/destination information with dotted access and pass it to a shared renderer, instead of rediscovering the variant inside an overbroad helper.Do not introduce a new class hierarchy or a general dispatch framework merely to remove one broad annotation.
5. Restore branch-specific types after dispatch and use dotted access.
In tal-repeat-next!, the heterogeneous sequence storage is legitimate, but the kind discriminator establishes whether the active value is a vector, string or Iterator. Introduce the corresponding
usingbinding with:-inside each established branch. Use a typed Iterator binding and its dottednext!call rather than carrying the value as:tthrough the implementation.In html/builder.ss:2262, the internal data is either a string or the accumulated list of pieces. Once the string branch is excluded, bind the other branch as
:listinstead of continuing to use an untyped value. Keep the representation invariant explicit; do not add redundant runtime checks for facts already established by the producer/dispatch.6. Avoid repeated checks in callbacks after the element type is established.
entry :- :pairrather than leave the entry untyped.:listalone does not validate their members. In a subsequent pass over the established attribute values, use:-annotations instead of checking the same member type again.Use the repository's one-argument-per-line signatures, aligned annotation sigils and typed dotted access consistently throughout these changes. Prefer an existing contract or a small shared procedure/macro over repeated validation loops and one-use wrappers.
Keep genuinely necessary broad types and semantic checks.
This is not a request to mechanically replace every
:tor delete every predicate. Arbitrary TAL expression values and exceptions, generic duplicate detection, heterogeneous scanner registers and polymorphic I/O adapters can need broad types. In particular,write-djot-htmlhas destination-dependent return behavior (string, bytes or the writer callback's result); do not blindly change it to return void or a fixnum. Preserve token-kind, UTF-8, namespace, nesting and serialization validation. Narrow types only when all call sites or a checked boundary establish the narrower contract.Validation scope: This follow-up is a source/type-contract audit. The proposed predicate-plus-base-type and
list-of?contract forms were checked in the available interpreter. No implementation changes were made, and the test/conformance suites were not rerun for this follow-up. Earlier runtime results apply to the earlier reviewed revision; the newest four-file commit was inspected as a diff.more changes requested, plus the shrink question about the utf8 encoding.
one way to deal with your buffer sizing is to extend the BufferWriter interface with a reserve method. then you can use memory buffers from stdio and use an definterface-method to call it directly when you need to reserve space. another thing you can do is use the buffer struct directly, it does already everything the ByteOutput does. probably the best course of action if you want to avoid interface overhead.
@vyzo latest feedback addressed!
From the clanker:
can you fix the open-detachable-writer bug while at it? i'll do another manual pass.
Follow-up review of
6c138fcb: request changes.The major ByteOutput and signature findings are addressed. Using stdio buffer structs directly matches the maintainer's subsequent guidance in comment 3100; the absence of a new BufferedWriter interface method is therefore not an outstanding finding.
1. P2 — Release temporary buffers when emission fails.
In markup/sink.ss:33,
buffer-detach!only runs afteremit!succeeds. An emission error or a sink failure during reservation bypasses cleanup. The textual-port branch at lines 55–58 has the same problem ifwriter.writeraises.I reproduced the emission-error path: the temporary buffer remains open, retains its cached allocation, and does not return it to the cache. Repeated failures discard reusable cache allocations and cause unnecessary allocation and GC pressure; this is not a claim of a file-descriptor leak.
Scope ownership of the temporary buffers explicitly. Flush and detach on success; discard pending bytes and release on failure. Preserve the original exception, including raised
#f, without retrying failed writes or closing the caller's sink. Simply placing the current flushing detach operation infinallywould introduce unwanted writes during failure cleanup. Keep this ownership handling shared between the affected paths rather than copying cleanup state machines.Add focused failure-path coverage for an emitter exception and an underlying write failure: the temporary allocation is released, the original exception is preserved, no failed write is retried, and the caller's sink remains open. Caller-owned BufferedWriter buffers are not temporary buffers and must remain caller-owned.
2. P2 — Validate fragment context before consuming the input.
read-html-fragment at html/reader.ss:48 still accepts
context : :tand constructs the tokenizer before checking the context.With an input port and
context: 42, I confirmed that the source is consumed completely beforeContractViolationis raised. For a streaming source this can also block waiting for EOF before reporting an invalid argument.Express the string-or-
XMLElementdomain in a predicate contract. Validate the context's tag name and namespace before reading the source. Keep those semantic checks; move them ahead of I/O. Add a regression check that an invalid context leaves an input port unread, and cover invalid context names/namespaces as well as an invalid argument type.3. P3 — Carry the established XML base type into internal node dispatch.
Both XML emit-node! and HTML emit-node! retain
node :- :t. Their node inputs have the commonXMLbase established by constructors, readers, or the public writer contracts.Use
node :- XMLin these dispatch procedures. Retain concrete subtype dispatch and rejection of node kinds unsupported by the output format: the base type does not establish that a node is writable in that format. The outer HTML entry point legitimately retains its node-or-list domain. This is a remaining precision/style cleanup, not a request to replace every polymorphic:tindiscriminately.The separately requested detachable-writer fix is still outstanding.
open-detachable-writer is unchanged, and calling it raises
non keyword slot initializer. This is pre-existing, rather than a regression in these commits, but the maintainer explicitly requested its repair in comment 3104. The matching reader constructor has the same positional-initializer pattern; address that shared mistake consistently and verify wrapping/detaching preserves ownership of the underlying object.Earlier findings resolved: the independent ByteOutput implementation is removed, constructor member-check loops have been replaced by contracts, the previously identified TAL/internal types have been narrowed, and the link/image rendering refactor shares the common work appropriately. I would close those earlier findings.
Validation: nine focused test modules passed. The pinned HTML conformance runs passed 7,045 tokenizer cases, 1,963 tree-construction cases, and 1,950 writer round-trip cases, with 8, 4, and 17 documented skips respectively and no failures. Eight targeted Djot link/image rendering cases matched the previous reviewed implementation. The cleanup and context-consumption findings above were reproduced separately.
git diff --checkpassed and no source files were changed.These checks used the reviewed sources through a temporary source overlay on the installed Gerbil runtime, not a full build of this revision. Compiled TAL coverage was skipped because the installed environment lacks its metadata, and the full Djot suite is blocked by the installed runtime's missing
char-set:punctuation. The targeted Djot renderer checks do not replace that suite.@vyzo addressed your feedback in the latest commits
Follow-up review of
af2fdbb0: approved.No new blocking findings. The outstanding findings from my previous review are addressed:
#f, and avoid retrying failed writes or closing caller-owned sinks.node :- XML, retaining the necessary subtype checks and rejection of node kinds unsupported by the output format.The fixes are appropriately small. The shared cleanup helper and single fragment-building call reduce duplication without adding unnecessary abstractions. I found no additional coding-convention or duplication issue warranting changes in this revision. The earlier ByteOutput consolidation and type-contract findings remain resolved; direct use of stdio buffer structs follows the maintainer's subsequent guidance.
Validation against this revision:
git diff --checkpassed. No source files were changed during review.These were source-overlay tests on the installed Gerbil runtime, not a full rebuild of this revision. The compiled TAL case was skipped because the environment lacks its metadata. The full Djot suite remains unverified in this environment because the installed runtime lacks
char-set:punctuation; it was not rerun in this pass. This approval does not claim full compiled-suite validation.astra is happy; i'll do another manual pass, and unless something iffy catches my eye, i'll merge.
well ok, i think it is ready to merge. a couple of minor comments, please use relative imports for package facades.
@ -8,0 +3,4 @@;;; Typed HTML: the HTML node types and checked constructors, the reader and;;; the writer. HTML trees are built from the node types of;;; :std/text/markup/xml as well. See html.md.(import :std/text/markup/html/typesuse relative imports with ./html/... its the package itself.
@ -15,0 +2,4 @@;;; © Gerbil contributors;;; Typed XML: node types and checked constructors, the reader and the;;; writer. See xml.md.(import :std/text/markup/xml/typesrelative imports please
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.