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

Optimizes normalize_headers for performance #1029

Conversation

baweaver
Copy link
Contributor

@baweaver baweaver commented Jun 6, 2023

Optimizes the WebMock::Util::Headers.normalize_headers method to:

  • Create less objects
  • Avoid using Regexp where possible
  • Hoists constants

More details can be found in the related issue, including performance benchmarks:

#1027

Optimizes the `WebMock::Util::Headers.normalize_headers` method to:

* Create less objects
* Avoid using Regexp where possible
* Hoists constants

More details can be found in the related issue, including performance
benchmarks:

bblimke#1027
@baweaver baweaver force-pushed the baweaver/performance/normalize_headers_optimization branch from 99924c2 to 1249b51 Compare June 6, 2023 16:53
[name.to_s.split(/_|-/).map { |segment| segment.capitalize }.join("-"),
case value

headers.each_with_object({}) do |(name, value), new_headers|

Choose a reason for hiding this comment

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

I'm not a maintainer, but each_with_object is an ActiveSupport method, and webmock doesn't depend on ActiveSupport. You'll probably need to change this for the test suite to pass.

Copy link
Contributor Author

@baweaver baweaver Jun 12, 2023

Choose a reason for hiding this comment

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

https://ruby-doc.org/3.2.2/Enumerable.html#method-i-each_with_object

Enumerable#each_with_object is in Ruby itself dating back several versions, confirmed that it's supported back to Ruby 1.9.3.

Also test suite was passing locally with this modification.

Choose a reason for hiding this comment

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

Wow. TIL. Thanks for the clarification!

@technicalpickles
Copy link
Contributor

Is there anything that can be done to help this along? I've been profiling our app's system specs (same one as @baweaver), and this is one I keep coming back to.

@@ -57,6 +60,15 @@ def self.basic_auth_header(*credentials)
"Basic #{Base64.strict_encode64(credentials.join(':')).chomp}"
end

def self.normalize_name(name)
Copy link
Owner

Choose a reason for hiding this comment

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

This code is very easy to read now. Thank you @baweaver !

@bblimke bblimke merged commit d4afbd9 into bblimke:master Aug 19, 2023
@technicalpickles
Copy link
Contributor

Thank you for merging 🙇🏻 Any chance we can get a release with it and #1033 🙏🏻

@bblimke
Copy link
Owner

bblimke commented Aug 26, 2023

@technicalpickles 3.19.0 is now released. thank you!

@baweaver baweaver deleted the baweaver/performance/normalize_headers_optimization branch August 29, 2023 20:24
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