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.

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

Add fast path for margin decoration access #179974

Merged
merged 2 commits into from
Apr 18, 2023
Merged

Conversation

joyceerhl
Copy link
Collaborator

@joyceerhl joyceerhl commented Apr 14, 2023

For #179725

@joyceerhl joyceerhl self-assigned this Apr 14, 2023
@joyceerhl joyceerhl requested a review from alexdima April 14, 2023 15:07
@joyceerhl joyceerhl marked this pull request as ready for review April 14, 2023 15:07
@joyceerhl joyceerhl enabled auto-merge (squash) April 14, 2023 15:07
@vscodenpa vscodenpa added this to the April 2023 milestone Apr 14, 2023
@joyceerhl joyceerhl disabled auto-merge April 14, 2023 15:25
@joyceerhl joyceerhl force-pushed the dev/joyceerhl/troubled-iguana branch from 69cfb38 to cc9e0ee Compare April 14, 2023 15:42
@joyceerhl joyceerhl force-pushed the dev/joyceerhl/troubled-iguana branch from cc9e0ee to adaad1a Compare April 14, 2023 15:47
@joyceerhl joyceerhl enabled auto-merge (rebase) April 14, 2023 18:17
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 Very nice!

I pushed a change where I remove both the onlyMinimapDecorations and onlyGlyphMargin flags from the caching logic in viewModelDecorations because the initial idea was that there are all these view parts that need to fetch the decorations that are in the viewport and all of these callers would benefit from this single cache, and the cache would be busted only once per frame.

But already with the addition of onlyMinimapDecorations from a few months ago (d53f4c5), this initial idea was no longer working properly, as the call from minimap.ts ended up breaking the cache due to the larger range (not just the viewport) the minimap uses.

So I made a special getter for the minimap call and I also left that the glyph margin view part ends up fetching all the decorations in the viewport and does filtering based on options.glyphMarginClassName in its implementation, as this allows it to get a cache hit for the viewport, just like everyone else.

@joyceerhl joyceerhl merged commit faefe8d into main Apr 18, 2023
@joyceerhl joyceerhl deleted the dev/joyceerhl/troubled-iguana branch April 18, 2023 12:53
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants