Cache common Primer values #197
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/view-components/9bflmlg23 |
BlakeWilliams
force-pushed
the
bmw-jk/classify-hashify
branch
from
3170e14
to
ed88642
Compare
* Add benchmark tests to avoid having to give imprecise counts due to differences in Ruby/Rails versions * Add CI for running benchmarks against Rails main, Ruby 2.7 * Only run simplecov if `COVERAGE=1` in your environment Co-authored-by: Justin Kenyon <kenyonj@github.com>
BlakeWilliams
force-pushed
the
bmw-jk/classify-hashify
branch
from
bee5c36
to
1f00b17
Compare
|
Here's the benchmark results as well:
|
|
|
||
| preload( | ||
| keys: [Primer::Classify::COLOR_KEY, Primer::Classify::BG_KEY], | ||
| values: [:blue, :gray_dark, :gray, :gray_light, :red, :orange, :orange_light, :yellow, :green, :purple, :white, :pink] |
There was a problem hiding this comment.
How did you arrive on these values here? (And for the rest of this file?) I'd love to have a process to ensure this optimization stays up to date 😄
There was a problem hiding this comment.
It was very unscientific, we went through https://primer.style/css/ and tried to find colors to fill in here 😅.
There's probably a few better ways to handle it, like maybe parsing primer itself for color variables that we could use but we didn't have time to look into anything like that.
Primer::Classify::Cacheclass for preloading common primer classesPrimer::Classify::Cachewhen available inPrimer::Classifydifferences in Ruby/Rails versions
COVERAGE=1in your environmentThis takes us from 114 allocations when not cached, down to 40 allocations when cached. There's probably a lot more work that could be done here, like catching new keys that
Classifysupports and tracking that we're caching them correctly.Co-authored with @kenyonj