Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Provide faster version of Formatter.Format that allows for skipping expensive recalculations of CSS #937

Closed
wants to merge 2 commits into from

Conversation

gkaikoponen
Copy link

@gkaikoponen gkaikoponen commented Feb 26, 2024

Formatter.Format currently spends most of its time (for small calls) reconstructing and re-compressing a CSS map from the given style, which in most cases will not be changing between calls to Format().

Expose public StyleToCSS() and FastFormat() methods that allow the caller to precompute this CSS map to improve performance.

This PR just proposes the simplest backwards-compatible change to allow the performance gains in question. A redesign of Formatter.Format might be called for, but memoizing the CSS map result may be expensive too if style names can't be assumed to uniquely identify an unmodified style.

Formatter.Format currently spends a lot of its time reconstructing and re-compressing a CSS map from the given style, which in most cases will not be changing between calls to Format().

Expose a FastFormat() method that allows the caller to precompute this CSS map to improve performance.
@alecthomas
Copy link
Owner

A couple of thoughts:

  1. Can you provide before/after benchmarks showing how much of an improvement this is? If it's a real problem, then this change sounds great.
  2. I'd suggest instead of adding this slightly awkward to use API we could introduce an internal LRU cache.

@alecthomas
Copy link
Owner

alecthomas commented Feb 26, 2024

memoizing the CSS map result may be expensive too if style names can't be assumed to uniquely identify an unmodified style

Style data structures are immutable and the pointer is obviously guaranteed to be unique, so that should suffice.

@gkaikoponen
Copy link
Author

According to pprof, for a use case calling Format once per line many times, (note this is on a pre-2.0 version but the result should be similar on latest), the original writeHTML takes ~350ms cumulative, while with FastFormat it takes ~50ms.

Good point, memoizing on pointer should be fine, I'll try a rework. My golang-fu is weak so let me know if I should be using something other than a hand-rigged map...

@gkaikoponen
Copy link
Author

gkaikoponen commented Feb 27, 2024

Updated to use a simple map with RW lock.
I'm having trouble determining what 'canonical' golang caching lib I should be using instead; golang-lru doesn't allow count-based expiry which I think is preferable to size-based expiry here; e.g. keep last 10 styles. groupcache is an awfully stale project. Let me know what you'd pick. Or we can just pray no one is passing dynamically-named styles.

@alecthomas
Copy link
Owner

NP thanks for your help, but I've whipped up a quick LRU cache that should be sufficient. I'll send a PR in a bit.

@alecthomas
Copy link
Owner

#938

I did this, not to step on your toes, I appreciate the contribution, but because your intuition that there are no small, simple solutions for LRU caches in Go is correct, and I wouldn't want to introduce a heavyweight dependency, and as you're new to Go I didn't want to burden you with having to come up with something that could be relatively complex.

Again, I appreciate the contribution.

@alecthomas
Copy link
Owner

Closed in favour of #938

@alecthomas alecthomas closed this Feb 27, 2024
@gkaikoponen gkaikoponen deleted the patch-1 branch February 27, 2024 16:04
@gkaikoponen
Copy link
Author

not to step on your toes

Absolutely no problem, I just just wanted to get the ball rolling with this PR, thanks for the quick response!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants