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

Improve performance to_h method of Aws::Rails::SqsActiveJob::Configuration object #108

Merged
merged 3 commits into from
Jan 10, 2024

Conversation

roharon
Copy link
Contributor

@roharon roharon commented Jan 5, 2024

Description of changes:

Improve performance to_h method of Aws::Rails::SqsActiveJob::Configuration object.

When erase @ of instance_variables,
it could be use String#delete instead of String#gsub .

delete is more faster than gsub
You can refer below benchmark.

require 'benchmark/ips'

STRING = "@abcdefg".freeze

def gsub
  STRING.gsub('@', '').to_sym
end

def delete
  STRING.delete('@').to_sym
end

Benchmark.ips do |x|
  x.report('gsub') { gsub }
  x.report('delete') { delete }
  x.compare!
end

the result is

ruby 3.2.2 (2023-03-30 revision e51014f9c0) [x86_64-linux]
Warming up --------------------------------------
                gsub    81.596k i/100ms
              delete   320.575k i/100ms
Calculating -------------------------------------
                gsub      1.230M (±13.2%) i/s -      6.120M in   5.099980s
              delete      3.425M (±12.5%) i/s -     16.990M in   5.067744s

Comparison:
              delete:  3425068.2 i/s
                gsub:  1229521.8 i/s - 2.79x  slower

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

@roharon roharon changed the title Modify erase @ of instance_variable with delete method instead of gsu… Improve performance to_h method of Aws::Rails::SqsActiveJob::Configuration object Jan 5, 2024
Copy link
Contributor

@alextwoods alextwoods left a comment

Choose a reason for hiding this comment

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

Nice - this looks good and thanks for the contribution + performance benchmarking!

@mullermp
Copy link
Contributor

mullermp commented Jan 6, 2024

Thanks. This needs a change log entry.

@roharon
Copy link
Contributor Author

roharon commented Jan 6, 2024

Thanks. This needs a change log entry.

Then, do I need to update CHANGELOG?

@mullermp
Copy link
Contributor

mullermp commented Jan 8, 2024

Yes, please update that file to add an issue line so that our automated system can release it.

@roharon
Copy link
Contributor Author

roharon commented Jan 9, 2024

@mullermp Thanks for your kindly guide. I changed it aaaf9ab

@mullermp
Copy link
Contributor

Thank you for your contribution! We will release this or next week, we have some upcoming operational work.

@mullermp mullermp merged commit 9218da1 into aws:main Jan 10, 2024
35 checks passed
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

3 participants