perf: pre-declare position on footnote factories#2
Open
ChristianMurphy wants to merge 1 commit intosyntax-tree:mainfrom
Open
perf: pre-declare position on footnote factories#2ChristianMurphy wants to merge 1 commit intosyntax-tree:mainfrom
ChristianMurphy wants to merge 1 commit intosyntax-tree:mainfrom
Conversation
Problem: the footnote handlers build footnoteReference and footnoteDefinition nodes as fresh literals, then mdast-util-from- markdown's enter() sets node.position afterwards. V8 treats that late assignment as a shape change, so each node walks through two hidden classes instead of one. Core nodes already avoid this by declaring position up front; without the same trick on extension nodes, GFM trees end up with mixed layouts and tree walks pay a polymorphic-dispatch cost. Goal: declare position on both footnote literals at construction so V8 keeps a single hidden class per node type. Changes: - lib/index.js: trailing position: undefined on the footnoteReference literal in enterFootnoteCall. - lib/index.js: trailing position: undefined on the footnoteDefinition literal in enterFootnoteDefinition. Notes: pairs with the matching core change in syntax-tree/mdast-util-from-markdown#53; merged on its own this branch is a no-op. Validated with npm test (build, format, 100% coverage, 22 of 22 tests in dev and prod). Bench (local harness, 3-run median-of-medians, GFM tokenizer and mdast extensions stacked). Numbers below are the parse time delta versus an unmodified upstream main; positive numbers mean slower, negative numbers mean faster. Input: a document with 1000 footnote definitions and 1000 references to them. On the baseline this scenario carries a modest mdast-layer share (full 78.0 ms, tokenize-only 62.4 ms) but every footnote node still pays the shape-change cost without the pre-declare. Effect of pairing this branch with PR #53: - PR #53 alone, extensions on upstream main: 19.5 percent slower. - PR #53 with this branch alongside (and the three sibling extension branches): 13.0 percent slower, which sits at the upper end of the bench's drift floor of 4 to 12 percent (main vs main with no code change in the same window). The paired configuration recovers about 6.5 percentage points relative to PR #53 on its own. Without this branch, PR #53 would appear to regress footnote-heavy parses; with it, most of that apparent regression goes away.
7bef571 to
4a85102
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Initial checklist
Description of changes
Problem: the footnote handlers build footnoteReference and
footnoteDefinition nodes as fresh literals, then mdast-util-from-
markdown's enter() sets node.position afterwards. V8 treats that
late assignment as a shape change, so each node walks through two
hidden classes instead of one. Core nodes already avoid this by
declaring position up front; without the same trick on extension
nodes, GFM trees end up with mixed layouts and tree walks pay a
polymorphic-dispatch cost.
Goal: declare position on both footnote literals at construction so
V8 keeps a single hidden class per node type.
Changes:
literal in enterFootnoteCall.
literal in enterFootnoteDefinition.
Notes: pairs with the matching core change in
syntax-tree/mdast-util-from-markdown#53;
merged on its own this branch is a no-op. Validated with npm test
(build, format, 100% coverage, 22 of 22 tests in dev and prod).
Bench (local harness, 3-run median-of-medians, GFM tokenizer and
mdast extensions stacked). Numbers below are the parse time delta
versus an unmodified upstream main; positive numbers mean slower,
negative numbers mean faster.
Input: a document with 1000 footnote definitions and 1000 references
to them. On the baseline this scenario carries a modest mdast-layer
share (full 78.0 ms, tokenize-only 62.4 ms) but every footnote node
still pays the shape-change cost without the pre-declare.
Effect of pairing this branch with PR #53:
branches): 13.0 percent slower, which sits at the upper end of the
bench's drift floor of 4 to 12 percent (main vs main with no code
change in the same window).
The paired configuration recovers about 6.5 percentage points
relative to PR #53 on its own. Without this branch, PR #53 would
appear to regress footnote-heavy parses; with it, most of that
apparent regression goes away.