v0.19: Add std/markup/djot #1509
No reviewers
mighty-gerbils/contributors
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
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
mighty-gerbils/gerbil!1509
Loading…
Reference in a new issue
No description provided.
Delete branch "jay/gerbil:v0.19-djot"
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?
Adds a native Djot parser and renderer under
:std/text/markup/djot.:std/ioinput/output.Benchmarks
Complete parsing into syntax trees, source locations disabled, no rendering. Gerbil compiled with
gxc -O -exe, measured on an AMD Ryzen 9 7940HS at commitbe13dea6.Median milliseconds per parse across five timed batches. File reading and process startup are excluded; garbage collection during parsing is included.
just a readability comment: can you ask astra to make the code sparser so that it is easier to read?
@vyzo wrote in #1509 (comment):
sure thing, done. still need to do some benchmarking and write up a description. you are welcome to look over the code though, it's basically done
WIP: v0.19: Add std/markup/djotto v0.19: Add std/markup/djotactually, now working on some more optimizations, i found this https://github.com/dcampbell24/djot-implementations/
and I see we are quite a bit behind the C parser in performance. unacceptable!!
ok, let me review and i can probably give some optimization advice. the gap to C is pretty big, but let's not throw safety out the window to close it. Also it provides a good benchmark we can use for the type-gerbil optimizer work.
so is the benchmark checked in? one obvious potential pitfall is using a port for the read; you should use directly a Reader, you can get one with
call-with-file-readerfrom std/io.we probably need an optimized writer for sxml, using a BufferedWriter instead of a port. maybe a bit out of scope for this pr, but definitely relevant to benchmarking.
S-expressions are not a very efficient data representation for HTML, either, if what you're looking for is performance.
first pass review, mainly looking at how we can improve the code and its performance:
@ -0,0 +96,4 @@(footnotes : :list))transparent: #t)(defstruct (djot-paragraph djot-container)you should mark the leaves of the AST with
final: #t. results in faster predicate and accessors/mutators.also note that transparent: #t is unnecessary, it is the default.
@ -0,0 +109,4 @@(if (or (= p end) (memq (attribute-parser-state parser) '(done fail))) p(let ((c (string-ref input p))(state (attribute-parser-state parser)))(case statethis could benefit from a hash table.
@ -0,0 +154,4 @@(extra : :list := [])) => :list(render-element tag node extra (children) newlines))(condthis is a big linear scan, i think it could benefit from an interface and dispatching the prerequisite methods. probably something to measure.
@ -0,0 +620,4 @@(def (inline-container (kind : :symbol)(source :? djot-source-range)(children : :list)) => djot-container(case kindthis could probably benefit from a hash table.
@ -0,0 +7,4 @@(export string->djot);;; Public Gambit Unicode character sets have no Gerbil wrapper in v0.19.(extern namespace: #f char-set:punctuation char-set-contains?)this should be added to the runtime module in the prelude.
@ -0,0 +114,4 @@candidate)))))))(def (visit (node : djot-node)) => :void(let (identifier (node-identifier node))you have a type annotation, use dotted notation!
i agree with fare; SSXML is ancient and not the fastest of cats. We could make a properly typed xml module, to replace sxml and html could be implemented using that. probably not in this pr though, it is already large, but it could be follow up work where we can measure improvements.
now, for immediate performance improvements see my comments, and also use a Reader instead of a port directly for reading the file to benchmark, this will make an immediate (but maybe small) difference.
v0.19: Add std/markup/djotto WIP: v0.19: Add std/markup/djotWIP: v0.19: Add std/markup/djotto v0.19: Add std/markup/djot@vyzo addressed your feedback, i kept the SXML intermediary for now
there is definitely a lot of meat left on the bones in terms of performance gains, though
yeah, but at least my quick feedback already produced gains; i looked in the benchmark.md and there seems to be quite an improvement -- clearly ahead of js now.
I think maybe we could improve performance by stopping parsing strings (and prereading a giant string) and just reading bytes/utf8 characters straight from the reader. Also it is likely that location tracking is expensive, we could have a flag to turn that off. It is useful for reporting errors, but it does cost, potentially quite a bit. Can you check with astra how hard it would be to make this transition to parsing from a buffered reader? Probably not so hard. This will also substantially improve memory usage as well, those big strings are heavy and memory hungry.
next we should figure out what to do with sxml; i think std/markup/xml and std/markup/html packages with properly structured xml and html are the way to go and now we have a nice benchmark we can use to measure things. But as I said this is for another pr, you want to take it?
@vyzo yeah I can take it, and you were thinking to remove sxml yes? like fully replace with std/markup/xml?
yea, sxml is ancient and not exactly a joy to work with.
@jay what do you think of trying the parser working directly with a (buffered) reader instead of parsing strings? We need to measure its performance, and we have benchmarks already.
@vyzo I'm working on addressing your feedback. I had astra try switching to buffered reader but did not see performance gains (~15–50% slower, although lower peak memory), but looking to see if there are additional optimizations
Ok, it was worth a try.
in this case for the benchmark just read the whole file at once with a buffer sized at the file size, read-all-from-reader does many small reads and then concatenates.
you can probably just use read-file-u8vector.
@vyzo done, benchmark now uses
read-file-u8vectorfor all file loadingmaybe we can pursue optimizing this more later, but the public API will be unchanged so this can unlock working on the documentation generator now. are you good with that?
yeah totally, lets make progress.
I'll do another pass, and @fare can point the clanker for the comprehensive review.
GPT 6.1 Sol Light says:
Reviewed head
ec648353. I found one parser regression and two renderer issues. The latter two also occur in pinned djot.js 0.3.2.[P2] Completing a reference link clears an enclosing destination.
inline.ss:393
When
[b][c]completes,finish-inline-link!unconditionally setsparser.destination?to#f. The outer)consequently cannot complete the original link.Upstream produces:
Preserve destination state when completing a reference link. Add this case and its image variant as regressions.
[P2] Generated and explicit attributes can produce duplicate HTML attributes.
html.ss:59–70
render-attributesappends both generated and explicit attributes, only special-casingclass. This produces twohrefattributes; browsers retain the first, ignoring the explicit override. Footnote references similarly permit duplicateid,hrefandrole.Ordinary links/images already filter these collisions in
render-reference. Apply a consistent precedence rule in the common attribute renderer and assert uniqueness in SXML as well as HTML.Unused footnotes can cause other unused footnotes to appear.
html.ss:621–625
Rendering every definition discovers
extrathrough the unused note, so “leaked” appears as footnote 2. Its backlink targetsfnref2, which occurs only in discarded output.Render definitions reachable from visible references, extending a queue as nested references are discovered. Mark notes before traversing them to handle cycles. If retaining upstream behavior is intentional, I’d track this as a follow-up.
The test coverage is substantial, and keeping the SXML replacement outside this PR seems sensible. I’d fix item 1 before merging. I ran the examples against upstream 0.3.2; the Gerbil findings are from source inspection because this environment has no Gerbil executable.
@fare I pushed a fix for 1
for 2 and 3 I am hesitant to fix those since they would be diverging from the "official" djot.js parser, but they do indeed look like bugs. I'll see about opening an issue on that repo, and if it does need fixing we can do that here later
I don't have strong opinions for what the "correct" behavior should be for the other issues, especially if upstream doesn't have their shit together. However, IF that is not too hard, it is better to issue an error when finding a badly defined situation than making up a bad answer, even one identical to upstream. i.e. I'd rather we err on the side of being too strict than that of spewing likely unintended nonsense as output.
@fare you were right, these were renderer bugs, not incorrect specs from the Djot spec. I compared
djot.jswithcdjotandJotdown, and neither has those bugs. Both issues are fixed now.LGT Sol 6.1 Light, that adds:
One documentation follow-up: update the Compatibility paragraph in djot.md to mention attribute deduplication and reachable-only footnote rendering, since both intentionally differ from djot.js 0.3.2.
semi-relatedly, what's the plan for extracting running tests from documentation examples?
alright lets merge to unblock documentation work.