Conversation
| } | ||
| fn uuids(len: usize) -> String { | ||
| (0..len) | ||
| .map(|_| "770f93e8-b4ee-4ce8-ab0f-4ece7d8c1090") |
There was a problem hiding this comment.
Hardcoding the same UUID seemed to reproduce the scaling behaviour, I left it like this to avoid adding another dependency.
| fn tiny_markdown_string(c: &mut Criterion) { | ||
| let doc = "A *single* [markdown](/path) string!".to_owned(); |
There was a problem hiding this comment.
I noticed a very small slowdown in this tiny test case (~8.9us -> ~9.1us; ~3%). This seems to even out very quickly as size grows. I tried creating an adaptive data structure that switched from Vec to BTreeMap at a certain size, but the complexity was quite high and the performance improvement was basically negligible.
| fn readme(c: &mut Criterion) { | ||
| let doc = fs::read_to_string("readme.md").unwrap(); |
There was a problem hiding this comment.
Criterion reported no difference for this benchmark.
| self.map | ||
| .sort_unstable_by(|a, b| a.0.partial_cmp(&b.0).unwrap()); |
There was a problem hiding this comment.
BTreeMap.iter() is sorted already
| while index < edit_map.map.len() { | ||
| if edit_map.map[index].0 == at { | ||
| edit_map.map[index].1 += remove; | ||
|
|
||
| match edit_map.map.get_mut(&at) { |
There was a problem hiding this comment.
I think was the source of the quadratic behaviour; beforehand we were iterating through the Vec to find elements, which is BTreeMap, we can get an element in
|
I'm not an knowledgable enough in this codebase to have opinions but just to wanted to say this looks like a great optimization 🙏 CI is still failing btw |
There was a problem hiding this comment.
Pull request overview
This PR addresses the scaling/performance issues reported in #113 by changing EditMap’s internal storage from a linear Vec to an ordered BTreeMap, and adds a benchmark that stresses large MDX/JSX expressions to measure the improvement.
Changes:
- Switch
EditMapfromVec-backed storage toBTreeMapto avoid O(n²) behavior when accumulating edits. - Update
EditMap::consumeto iterate edits in key order (and reverse key order for application) without sorting. - Add Criterion benchmarks (including a large JSX-expression case) and add
itertoolsas a dev dependency for string generation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/util/edit_map.rs |
Replaces the edit record with a BTreeMap and updates edit accumulation/application logic accordingly. |
benches/bench.rs |
Adds new benchmarks, including a large JSX/MDX-style stress case for parser scaling. |
Cargo.toml |
Adds itertools as a dev-dependency to support benchmark string generation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Other than the Clippy lint, I believe CI is failing because our version of I tried upgrading |
|
What's the holdup on this? Is there anything I can do to help? For my use case, the runtime of a tool I have that checks links in markdown files, went from around 2 minutes, to 2 seconds, with this pr, with exactly the same behavior. |
|
Glad to hear it worked well! I imagine the maintainers are busy and this isn't the highest priority thing on their plate. To maintainers: If you have a working lockfile, feel free to push it over mine to make review easier. |
|
friendly ping @wooorm |
I believe this fixes #113 by switching
EditMapfrom aVecto aBTreeMap.I created a version of @robsimmons benchmark and added it to
benches. Here's the result of running thelarge_jsx_expressionsbenchmark against the main branch (Vec) and myBTreeMapimplementation (this PR), varying thenum_jsx_lines_per_componentvariable.