-
Notifications
You must be signed in to change notification settings - Fork 50
perf(parse): share the configured markdown-it instance between parsers #407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
benjamincanac
wants to merge
18
commits into
main
Choose a base branch
from
perf/share-parser
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+99
−4
Open
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
1a12d12
perf: share one parser across callers with equivalent options
benjamincanac b48e98c
fix(vue): accept a parsed document in a defined component (#408)
benjamincanac 149406e
Merge remote-tracking branch 'origin/main' into HEAD
benjamincanac 01210c6
revert: drop the opt-in document cache
benjamincanac 1aa07d5
revert: move the serialized task rejection fix to its own PR
benjamincanac 1689816
fix(svelte,angular): keep one parser per streaming instance, reparse …
benjamincanac e328c4e
fix(parse): make the parser key unambiguous for arrays
benjamincanac 12aced4
docs: note that parser overrides options and plugins
benjamincanac babf0cd
chore: refresh the bundle size snapshot
benjamincanac 119133c
perf(parse): share the configured markdown-it instance between parsers
benjamincanac 64bf0c9
revert(vue,react,svelte,angular): drop the parser registry integration
benjamincanac 27c0a56
docs: describe parser construction cost
benjamincanac 3311978
chore: refresh the bundle size snapshot
benjamincanac 2a6e5a0
test(parse): drop the wall-clock assertion from the memo test
benjamincanac 9be2723
refactor(parse): simplify sharing the markdown-it instance
benjamincanac f7e62ea
Merge remote-tracking branch 'origin/main' into perf/share-parser
benjamincanac 1028a5a
chore: refresh the bundle size snapshot
benjamincanac f863c47
refactor(parse): shorten the shared instance lookup
benjamincanac File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { createMarkdownParser, defineComarkPlugin } from 'comark' | ||
| import type { MarkdownItPlugin } from 'comark' | ||
|
|
||
| // The shared instance is internal, so it is observed through the public API: a | ||
| // markdown-it plugin function runs once per instance, so counting how often it | ||
| // is registered counts the instances that were built. | ||
| let stableUses = 0 | ||
| const stableMdPlugin = (() => { | ||
| stableUses++ | ||
| }) as unknown as MarkdownItPlugin | ||
|
|
||
| const stablePlugin = defineComarkPlugin(() => ({ | ||
| name: 'sharing-stable', | ||
| markdownItPlugins: [stableMdPlugin], | ||
| })) | ||
|
|
||
| let closureUses = 0 | ||
| const closurePlugin = defineComarkPlugin(() => ({ | ||
| name: 'sharing-closure', | ||
| markdownItPlugins: [ | ||
| (() => { | ||
| closureUses++ | ||
| }) as unknown as MarkdownItPlugin, | ||
| ], | ||
| })) | ||
|
|
||
| describe('parser sharing', () => { | ||
| it('builds one instance for parsers with the same plugin functions', () => { | ||
| const plugin = stablePlugin() | ||
| const before = stableUses | ||
|
|
||
| for (let i = 0; i < 20; i++) { | ||
| createMarkdownParser({ plugins: [plugin] }) | ||
| } | ||
|
|
||
| expect(stableUses - before).toBe(1) | ||
| }) | ||
|
|
||
| it('builds one instance per closure when a factory returns a fresh function', () => { | ||
| const before = closureUses | ||
|
|
||
| for (let i = 0; i < 5; i++) { | ||
| createMarkdownParser({ plugins: [closurePlugin()] }) | ||
| } | ||
|
|
||
| expect(closureUses - before).toBe(5) | ||
| }) | ||
|
|
||
| it('does not share an instance between linkify settings', async () => { | ||
| const withLinkify = await createMarkdownParser({ linkify: true })('See https://comark.dev for more') | ||
| const withoutLinkify = await createMarkdownParser({ linkify: false })('See https://comark.dev for more') | ||
|
|
||
| expect(JSON.stringify(withLinkify.nodes)).toContain('"a"') | ||
| expect(JSON.stringify(withoutLinkify.nodes)).not.toContain('"a"') | ||
| }) | ||
|
|
||
| it('keeps streaming state on the parser across another parser use', async () => { | ||
| const streaming = createMarkdownParser() | ||
| const other = createMarkdownParser() | ||
|
|
||
| await streaming('# Title\n\nFirst paragraph.\n', { streaming: true }) | ||
| const second = await streaming('# Title\n\nFirst paragraph.\n\nSecond paragraph.\n', { streaming: true }) | ||
|
|
||
| await other('Unrelated **document**') | ||
|
|
||
| const third = await streaming('# Title\n\nFirst paragraph.\n\nSecond paragraph.\n\nThird paragraph.\n', { | ||
| streaming: true, | ||
| }) | ||
|
|
||
| // Reused nodes are carried over by reference from the previous output. | ||
| expect(third.nodes[0]).toBe(second.nodes[0]) | ||
| expect(third.nodes[1]).toBe(second.nodes[1]) | ||
| expect(third.nodes).toHaveLength(4) | ||
| }) | ||
| }) |
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a bound to the shared parser cache.
sharedParsersretains every unique plugin-function key permanently. A caller that repeatedly creates parsers with fresh plugin closures causes unbounded memory growth.Restore bounded eviction while retaining the shared-instance optimization.
Proposed fix
+const MAX_SHARED_PARSERS = 32 const sharedParsers = new Map<string, MarkdownExit>()🤖 Prompt for AI Agents