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

Cache image list markers #303

Merged
merged 1 commit into from
Apr 4, 2024
Merged

Conversation

erudel
Copy link
Collaborator

@erudel erudel commented Apr 4, 2024

Problem: when an editor contains many list items that are represented by the same image marker, there is a substantial amount in the layout pass spent resizing the image to the given size.

Solution: since the markers and sizes are the same for list items, we can cache the resized image and reuse it for successive calls. In some cases the number of calls to resize images went from ~7700 to just 4.

The implementation uses a Dictionary to store a cache for each image, and the cache is keyed by the size. I tried creating a cache with a single key derived from the image and size, however the string interpolation turned out to be more expensive than this approach.

I profiled the rendering before/after the change and here are the results:

Before:
before cache

After:
after cache

@@ -410,7 +427,7 @@ class LayoutManager: NSLayoutManager {
let attributes = lineNumberAttributes(lineNumberFormatting: lineNumberFormatting)
let text = NSAttributedString(string: "\(lineNumber)", attributes: attributes)
let markerSize = text.boundingRect(with: .zero, options: [], context: nil).integral.size
var markerRect = self.rectForLineNumbers(markerSize: markerSize, rect: rect, width: gutterWidth)
let markerRect = self.rectForLineNumbers(markerSize: markerSize, rect: rect, width: gutterWidth)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There was a warning for this variable not being changed after assignment, so I changed it to be a let constant

let sizeCache = imagecache[image, default: .init()]
imagecache.updateValue(sizeCache, forKey: image)

let key = size.debugDescription as NSString
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm open to suggestion on how to further optimize the keying strategy. With this change, the most expensive operation becomes generating the string description for the size (the whole 4ms in the screenshot above). Interpolating the image and size in a single string was coming up at 11ms, which is still better than the original 650ms, but I ended choosing the fastest version.

Copy link
Owner

Choose a reason for hiding this comment

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

this seems pretty good @erudel. We can evaluate if we can use a better strategy than this for generating the key but this in itself seems like a substantial improvement. Thank you for taking this up.

@rajdeep rajdeep merged commit 7a2e13e into rajdeep:main Apr 4, 2024
4 of 5 checks passed
@rajdeep rajdeep mentioned this pull request Apr 4, 2024
@erudel erudel deleted the list-marker-image-cache branch April 4, 2024 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants