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 test to limit allocations of Primer::Classify #166

Merged
merged 20 commits into from Jan 27, 2021
Merged

Conversation

BlakeWilliams
Copy link
Contributor

@BlakeWilliams BlakeWilliams commented Jan 26, 2021

Reduce allocations from 97 to 38 when using a "basic" Primer::Classify#call implementation.

closes part of #155

@vercel
Copy link

vercel bot commented Jan 26, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/view-components/eakj5jko7
✅ Preview: https://view-components-git-bmw-allocations.primer.vercel.app

@vercel vercel bot temporarily deployed to Preview January 26, 2021 16:50 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 16:53 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 16:59 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 17:04 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 17:10 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 17:14 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 17:33 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 17:35 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 17:42 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 17:48 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 18:09 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 18:21 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 18:41 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 19:06 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 19:08 Inactive
@vercel vercel bot temporarily deployed to Preview January 26, 2021 19:17 Inactive
@BlakeWilliams
Copy link
Contributor Author

I ran some benchmarks testing the new classify vs the classify on main and it looks like we're ~1.6x faster too!

primer_view_components λ bundle exec rake benchmark
/Users/blake/.rbenv/versions/2.7.1/bin/ruby ./performance/benchmark.rb
Warming up --------------------------------------
            classify     5.291k i/100ms
        old classify     3.188k i/100ms
Calculating -------------------------------------
            classify     51.519k (± 6.5%) i/s -    259.259k in   5.053442s
        old classify     32.399k (± 4.7%) i/s -    162.588k in   5.029117s

Comparison:
            classify:    51519.4 i/s
        old classify:    32399.3 i/s - 1.59x  (± 0.00) slower

@BlakeWilliams BlakeWilliams marked this pull request as ready for review January 26, 2021 19:20
memo
end

def extract_value(memo, key, val, breakpoint)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept these names the same as from before the refactor, but happy to update them to be more descriptive.

lib/primer/classify.rb Show resolved Hide resolved
lib/primer/classify.rb Show resolved Hide resolved
test/primer/classify_test.rb Outdated Show resolved Hide resolved
values = { align_self: :center, width: :fit, p: 4, m: 1, border: :top, box_shadow: true, color: :red, visibility: :hidden}
Primer::Classify.call(**values)

assert_allocations 38, within: 4 do
Copy link
Contributor

Choose a reason for hiding this comment

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

Where'd this value of 4 come from? Does this value vary a little in local test vs. CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should probably add a comment, but I believe the difference is between Ruby and Rails versions, since we run a matrix of versions in CI.

The other thought was to make a build specifically for testing allocations against 2.7, but I thought I'd submit this as-is and get thoughts/feedback first.

@vercel vercel bot temporarily deployed to Preview January 27, 2021 14:55 Inactive
@BlakeWilliams
Copy link
Contributor Author

Updated the test to include all, or most of the keys we support. Running the test against main resulted in 204 allocations, this branch resulted in 87 allocations.

Call val.to_s.dasherize only when necessary
Replace Hash#except with mutation
Use mutation on `#call` hash instead of `Hash#except`
Convert styles to a string instead of using an array
Replace `.chars.last` with `[-1]`
Separate single value conversion from breakpoint conversion
* change from using regex to using ord comparison
* misc. unrelated changes
@vercel vercel bot temporarily deployed to Preview January 27, 2021 15:04 Inactive
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.

None yet

4 participants