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 default MAX_RAND value for length 16 & 32 #1693

Merged
merged 2 commits into from
Dec 12, 2022
Merged

Conversation

tungmq
Copy link
Contributor

@tungmq tungmq commented Dec 10, 2022

Before contributing, please read our contributing guidelines and code of conduct.

Overview

  • I ran two cases:
    The first one is for default values that we're using in codebase (length 16 & 32).
    The second one is custom length (value: 15).

  • Benchmark result

Benchmark.ips do |x|
  x.report("generate_guid") {
    generate_guid
  }
  x.report("generate_guid_new") {
    generate_guid_new
  }
  x.compare!
end
----------------------------------------------------------------------------
Warming up --------------------------------------
       generate_guid   300.331k i/100ms
   generate_guid_new   394.048k i/100ms
Calculating -------------------------------------
       generate_guid      3.005M (± 0.1%) i/s -     15.317M in   5.097986s
   generate_guid_new      3.944M (± 0.4%) i/s -     20.096M in   5.095270s

Comparison:
   generate_guid_new:  3944187.7 i/s
       generate_guid:  3004503.1 i/s - 1.31x  (± 0.00) slower

Benchmark.ips do |x|
  x.report("generate_guid with custom length") {
    generate_guid(15)
  }
  x.report("generate_guid_new with custom length") {
    generate_guid_new(15)
  }
  x.compare!
end
----------------------------------------------------------------------------
Warming up --------------------------------------
generate_guid with custom length
                       574.869k i/100ms
generate_guid_new with custom length
                       541.947k i/100ms
Calculating -------------------------------------
generate_guid with custom length
                          5.731M (± 0.5%) i/s -     28.743M in   5.015260s
generate_guid_new with custom length
                          5.414M (± 0.2%) i/s -     27.097M in   5.005517s

Comparison:
generate_guid with custom length:  5731344.7 i/s
generate_guid_new with custom length:  5413515.2 i/s - 1.06x  (± 0.00) slower

Submitter Checklist:

  • Include a link to the related GitHub issue, if applicable
  • Include a security review link, if applicable

Testing

The agent includes a suite of unit and functional tests which should be used to
verify your changes don't break existing functionality. These tests will run with
GitHub Actions when a pull request is made. More details on running the tests locally can be found
here for our unit tests,
and here for our functional tests.
For most contributions it is strongly recommended to add additional tests which
exercise your changes.

Reviewer Checklist

  • Perform code review
  • Add performance label
  • Perform appropriate level of performance testing
  • Confirm all checks passed
  • Add version label prior to acceptance

@CLAassistant
Copy link

CLAassistant commented Dec 10, 2022

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the community To tag external issues and PRs submitted by the community label Dec 10, 2022
We use Rubocop as our linter. This commit addresses a warning under the Layout/EndAlignment cop.
Copy link
Contributor

@hannahramadan hannahramadan left a comment

Choose a reason for hiding this comment

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

Hi tungmq! This looks great and we appreciate the performance benchmarks you shared. I made a small formatting edit to make our linter happy. Thank you for your contribution!

@hannahramadan hannahramadan merged commit 0b89d57 into newrelic:dev Dec 12, 2022
@fallwith
Copy link
Contributor

Hi @tungmq. Thanks for the performance boost! This change has gone out in v8.14.0 of the newrelic_rpm gem and you have been recognized as a contributor in our CHANGELOG!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community To tag external issues and PRs submitted by the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants