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

Use CamelCase in handlers for less memory, more speed #340

Closed
wants to merge 1 commit into from

Conversation

jmondo
Copy link
Contributor

@jmondo jmondo commented Oct 16, 2016

gsubbing the handler pattern strings from snake case to camel case uses a lot of unnecessary memory. profiler coming in the comments.

@jmondo
Copy link
Contributor Author

jmondo commented Oct 16, 2016

I added this test:

def test_one_thing_memory
  report = MemoryProfiler.report do
    time = Chronic.parse("2012-08-02T13:00:00")
    assert_equal Time.local(2012, 8, 2, 13), time
  end
  report.pretty_print(to_file: 'memory.log')
end

@jmondo
Copy link
Contributor Author

jmondo commented Oct 16, 2016

memory-before.txt
memory-after.txt

handler.rb 72,736 -> 5,120
chronic/lib 167,181 -> 94,445

@jmondo
Copy link
Contributor Author

jmondo commented Oct 16, 2016

For benchmarking I ran the test suite 5 times on master and this branch

master
Finished in 2.317107s, 71.6411 runs/s, 308.1429 assertions/s.
Finished in 2.267655s, 73.2034 runs/s, 314.8626 assertions/s.
Finished in 2.133667s, 77.8003 runs/s, 334.6351 assertions/s.
Finished in 2.184228s, 75.9994 runs/s, 326.8890 assertions/s.
Finished in 2.094341s, 79.2612 runs/s, 340.9187 assertions/s.
(average 2.1993s)

camelcase
Finished in 2.096705s, 79.1719 runs/s, 340.5344 assertions/s.
Finished in 1.995551s, 83.1850 runs/s, 357.7959 assertions/s.
Finished in 2.004112s, 82.8297 runs/s, 356.2675 assertions/s.
Finished in 2.016496s, 82.3210 runs/s, 354.0796 assertions/s.
Finished in 2.024613s, 81.9910 runs/s, 352.6600 assertions/s.
(average 2.0274s)

(-7.81%)

faster 👍

@jmondo
Copy link
Contributor Author

jmondo commented Oct 16, 2016

let me know if this is too ugly. i have some other nicer looking but slightly less performant solutions. (still an improvement, though)

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.

1 participant