Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use struct version of DocumentMappingService APIs #9284

Closed
davidwengier opened this issue Sep 14, 2023 · 1 comment
Closed

Use struct version of DocumentMappingService APIs #9284

davidwengier opened this issue Sep 14, 2023 · 1 comment
Labels
perf 📊 tech-debt
Milestone
17.9 Planning

Comments

@davidwengier
Copy link
Contributor

davidwengier commented Sep 14, 2023 •
edited
Loading

PR #9285 introduced new APIs for the document mapping service that don't allocate for their outputs, and try not to allocate for intermediate state. We should go through existing callers of the old API and consider moving them where appropriate. If all that is happening is mapping being done from LSP types, and being sent back over LSP, then it doesn't matter, but if there is internal state being manipulated it would be a good idea.

Semantic Tokens was done in that same PR, as an example.
@ghost ghost added the untriaged label Sep 14, 2023
@davidwengier davidwengier mentioned this issue Sep 14, 2023
Merged
davidwengier added a commit that referenced this issue Sep 14, 2023
@davidwengier
Further reduce allocations in semantic tokens (#9285)
c3b3fab 
Follow up to #9280

This creates a struct based API for document mapping, and moves semantic
tokens to it. I did it in a source-compatible way so we can upgrade
existing features as necessary. I suspect most won't get the benefit
that semantic tokens gets, as most things just ferry ranges and
positions around, and don't process them much. Logged
#9284 to follow up though.

Commit-at-a-time might be easiest.

Results for semantic tokens are pretty good though:

![image](https://github.com/dotnet/razor/assets/754264/7316e5db-0e90-4b32-a807-d4ee5d40741a)
@phil-allen-msft phil-allen-msft added this to the 17.9 Planning milestone Sep 21, 2023
@ghost ghost removed the untriaged label Sep 21, 2023
@davidwengier
Copy link
Contributor Author

I looked, the only endpoint that calls the service for internal data only is the spell checking endpoint, and that is only used for verification. By virtue of the code using var this was updated automatically when the API was changed.

So going to close this as done.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
perf 📊 tech-debt
Projects
None yet
Milestone
17.9 Planning
Development

No branches or pull requests
2 participants
@davidwengier @phil-allen-msft