perf: avoid repeated raw string copies in nested lists - #4082
Conversation
Tokenizer.list() built each item's raw by appending line by line, and the list's raw by concatenating its items. src is only ever consumed from the front, so both are contiguous spans of the input and can be sliced out of it instead. V8 keeps a slice as a view on one backing string rather than a fresh copy per list and per item. Token values are unchanged. On cmark's pathological "deeply nested lists" case (500 levels): 222 MB RSS and 135 ms before, 157 MB and 126 ms after. No change on the CommonMark spec corpus -- the saving is proportional to nesting depth, so ordinary documents are unaffected.
|
@unshorn is attempting to deploy a commit to the MarkedJS Team on Vercel. A member of the Team first needs to authorize it. |
unshorn
changed the title
|
What is the benefit of this change? Seems like it complicates the logic to the point that any change in the future could break things with no up side unless someone is trying to nest 500 lists, which seems very impractical. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Thank you for reviewing the request. the performance benefits are only seen in the pathological cases (deeply nested lists).. unfortunately, it doesn't entirely eliminate the bug class as pushing the depth can still result in heap exhaustion. this will require more than a patch. As for the cost/benefit of the refactoring w.r.t. maintenance, I'll leave that to your judgement. I think there's less of a burden on maintaining Kind regards. |
There was a problem hiding this comment.
Reviewed d7e1b92ed053451a3585d943323f7f858b780fd5 against base 63841d3b0368e4b8df025ac3aca699141fa7bc56.
The consumed-source bounds preserve the raw-token contract in an independent comparison: 22,160 unique inputs across five GFM/pedantic/breaks configurations, with zero differences in complete lexer token trees (including raw values and link maps) or rendered HTML across 110,800 comparisons. Inputs include 850 unique existing fixtures, exhaustive three-line combinations of list markers/blank lines/tabs/fences/definitions, missing final newlines, and nested lists. This directly checks the synthetic-final-newline assumption in the new comment.
I also reproduced the memory benefit locally on Node 24.15.0, using a fresh process for each run and alternating base/head order. For a 500-level list over five runs each, median peak RSS was 198.5 MiB on base versus 124.3 MiB on head; median parse time was 312.1 ms versus 280.8 ms. All ten outputs have the same SHA-256. These are measurements for that pathological input on this machine, not a general throughput guarantee or a new stack-depth claim.
Full npm test passes, including specs, unit tests, docs/build, UMD/CJS, type/package checks and lint. All exposed upstream checks are successful apart from the expected skipped Release job. No author branch was edited.
Review and local validation performed with Codex.
3 participants
Marked version:
18.0.11 /
63841d3bMarkdown flavor: CommonMark
Description
Tokenizer.list()builds each item'srawa line at a time (raw += rawLine + '\n') and thelist's
rawby concatenating its items.srcis only ever consumed from the front, so both arecontiguous spans of the input; this computes their bounds and slices them out instead. V8 keeps a
slice as a view onto one backing string, so the per-item and per-list copies go away. Token values
are unchanged.
Local measurements from my machine:
deeply nested listspathological case (500 levels): 222-226 MB RSS and 132-140 msbefore, 157 MB and 124-127 ms after, across four rounds. The rest of cmark's pathological corpus
is unchanged.
npm run bench: unchanged, 1278/1319/1320 ms before against 1276/1312/1302 ms after. The savingis proportional to nesting depth, so ordinary documents get no marked benefit from the improvement.
RangeError: Maximum call stack size exceededis 2237 both beforeand after, so there is no stack cost.
blockquote()does not accumulate line by line, and its continuation paths splice a re-lexednewToken.rawinto the middle ofraw, so it is not expressible as one slice. Left alone.Contributor
covering this PR); or,
a new feature.
Covered by the existing list coverage in
test/specs/commonmark,test/specs/newandtest/unit/Lexer.test.js; fullnpm testpasses locally. I also diffed this branch againstmasteron the full token tree and rendered HTML for every input intest/specsacross five optionsets, and on 40,000 seeded-random list documents across three and observed no differences in either run.
Committer
In most cases, this should be a different person than the contributor.