Skip to content

Reuse display_map snapshot where possible to reduce snapshot creation - #30840

Closed
RemcoSmitsDev wants to merge 12 commits into
zed-industries:mainfrom
RemcoSmitsDev:project-search-performance-improvements
Closed

Reuse display_map snapshot where possible to reduce snapshot creation#30840
RemcoSmitsDev wants to merge 12 commits into
zed-industries:mainfrom
RemcoSmitsDev:project-search-performance-improvements

Conversation

@RemcoSmitsDev

@RemcoSmitsDev RemcoSmitsDev commented May 16, 2025

Copy link
Copy Markdown
Collaborator

While debugging the slow performance of the project search, I discovered that in several cases we were creating multiple display_map snapshots within the same root-level function call. I noticed that creating a display_map snapshot is quite slow, and found that in some cases we were generating the snapshot multiple times.

Release Notes:

  • N/A

While I was debugging why the project search was slow I discoverd that
we where making in a few cases multiple display_map snapshot within the
same function root caller. This also popped up inside my instruments
capture.
@RemcoSmitsDev
RemcoSmitsDev marked this pull request as draft May 16, 2025 18:43
@maxdeviant maxdeviant changed the title perf: Reuse display_map snapshot where possible to reduce snapshot creation Reuse display_map snapshot where possible to reduce snapshot creation May 16, 2025
@RemcoSmitsDev
RemcoSmitsDev marked this pull request as ready for review May 17, 2025 10:26
@RemcoSmitsDev
RemcoSmitsDev marked this pull request as draft May 17, 2025 10:38
@notpeter notpeter added the cla-signed The user has signed the Contributor License Agreement label May 17, 2025
@RemcoSmitsDev
RemcoSmitsDev marked this pull request as ready for review May 17, 2025 17:04
@ConradIrwin

Copy link
Copy Markdown
Member

Thanks for this!

I think the change is reasonable (in particular passing a display map instead of a cx to get selections makes sense). It obviously conflicts with a lot though, sorry for not reviewing faster.

That said, I'm curious to see some numbers. Do you have a reproduction case in search and how much faster does this make it? (I also wonder if there's more we can do to make creating snapshots faster... cc @maxbrunsfeld).

@RemcoSmitsDev

RemcoSmitsDev commented Jun 3, 2025

Copy link
Copy Markdown
Collaborator Author

Hey, thanks for the review and no worries!

I use the vscode-js-debug repo for project search testing because it gets some pretty large minified JavaScript files after a bun install & bun run compile. I use the search term ignore to get a decent amount of results.

What would be the best way to measure this? This PR should also improve the performance of some vim code paths that use selections, but that might be harder to measure, I guess.

Yeah, we might be able to improve the performance there, but hard to say because I'm not familiar with the code.

@RemcoSmitsDev
RemcoSmitsDev force-pushed the project-search-performance-improvements branch from a219635 to 6d69936 Compare June 3, 2025 09:13
@ConradIrwin

Copy link
Copy Markdown
Member

The way I've typically tested project search speed (though it isn't very scientific) is to set a global static when we start the search, and print out time elapsed when the 1st, 10th, 100th result has been added to the multibuffer.

For large minified files, I wonder if we aren't also running into similar issues as #16120...

@RemcoSmitsDev

Copy link
Copy Markdown
Collaborator Author

Yeah, I think we can tackle that later as Anthony is fixing the tabmap performance issue.

Comment thread crates/agent/src/agent_diff.rs Outdated
@RemcoSmitsDev

RemcoSmitsDev commented Jun 6, 2025

Copy link
Copy Markdown
Collaborator Author

@ConradIrwin So my measurement does not say it's faster, which makes sense in a way because I'm not hitting the code path that originally created 2 snapshots instead of one. This one happens when line_mode is enabled, in the case of getting the newest_adjusted selection, for example. But for multiple vim code paths, we now only create one display snapshot, and each snapshot creation takes about ~300ms. I'm not a vim person, so it's hard to measure what the exact performance gain is there, but should be a ~2x.

Before: (average 224.5260415ms)
Search operation completed in 239.47475ms
Search operation completed in 216.144541ms
Search operation completed in 221.07775ms
Search operation completed in 221.407125ms

After: (average 226.93803125ms)
Search operation completed in 230.300875ms
Search operation completed in 240.185833ms
Search operation completed in 215.665708ms
Search operation completed in 221.599709ms

@RemcoSmitsDev
RemcoSmitsDev force-pushed the project-search-performance-improvements branch from 2c27403 to e6faad4 Compare June 6, 2025 20:48
@RemcoSmitsDev
RemcoSmitsDev force-pushed the project-search-performance-improvements branch from 4ea0052 to aa77f43 Compare June 6, 2025 20:54
@RemcoSmitsDev
RemcoSmitsDev requested a review from ConradIrwin June 7, 2025 17:45
@notpeter

notpeter commented Aug 6, 2025

Copy link
Copy Markdown
Contributor

What's the status of this? (branch has gone stale and has merge conflicts)
@RemcoSmitsDev are you waiting for a review from @ConradIrwin?

@RemcoSmitsDev

RemcoSmitsDev commented Aug 6, 2025

Copy link
Copy Markdown
Collaborator Author

if we want to keep go forward with this pr happy to fix the big merge conflicts. After that might need a review

@ConradIrwin

Copy link
Copy Markdown
Member

I don't think this is going to be easy to land at all.

Closing for now, but we can come back to it later

@ConradIrwin ConradIrwin closed this Sep 9, 2025
nia-e pushed a commit that referenced this pull request Oct 17, 2025
Re-applies #30840

This PR re-applies the initial
[PR](#30840). As it was closed
because it was hard to land, because of the many conflicts. This PR
re-applies the changes for it.

In several cases we were creating multiple display_map
snapshots within the same root-level function call.
Creating a display_map snapshot is quite slow, and in some
cases we were creating the snapshot multiple times.

Release Notes:

- N/A
jasonsmithio pushed a commit to paddleboarddev/paddleboard that referenced this pull request May 31, 2026
Re-applies zed-industries/zed#30840

This PR re-applies the initial
[PR](zed-industries/zed#30840). As it was closed
because it was hard to land, because of the many conflicts. This PR
re-applies the changes for it.

In several cases we were creating multiple display_map
snapshots within the same root-level function call.
Creating a display_map snapshot is quite slow, and in some
cases we were creating the snapshot multiple times.

Release Notes:

- N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed The user has signed the Contributor License Agreement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants