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

Refactor Time methods #6581

Merged

Conversation

straight-shoota
Copy link
Member

This PR refactors Time.year_month_day_year and Time.absolute_days to simplify the code and make it easier to understand.

The changes are mostly just formal, but there are a few semantic changes such as storing the value of an expression in a local variable for re-use.

src/time.cr Outdated Show resolved Hide resolved
Copy link
Contributor

@RX14 RX14 left a comment

Choose a reason for hiding this comment

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

Looks good, sorry to have to ask but do you have any performance benchmarks?

@straight-shoota
Copy link
Member Author

year_month_day_day_year is about 25% faster than before.

I actually didn't expect much change in absolute_days but for some reason it's about 10% slower. But the measurements vary a lot, it's hard to nail it down, but that seems to be the trend.
When I change the operations back exactly how it was before, it somehow measures about 10% improvement.
This is all really weird and inconclusive. Can someone verify that?

This is my benchmark, it's running through the entire date range, so maybe a bit overkill...

require "benchmark"

struct Time
  Benchmark.ips do |bm|
    bm.report "new impl" do
      (1..9999).each do |year|
        ordinal = 0
        (1..12).each do |month|
          (1..days_in_month(year, month)).each do |day|
            ordinal += 1
            Time.absolute_days(year, month, day)
          end
        end
      end
    end
    bm.report "old impl" do
      (1..9999).each do |year|
        ordinal = 0
        (1..12).each do |month|
          (1..days_in_month(year, month)).each do |day|
            ordinal += 1
            Time.absolute_days_old(year, month, day)
          end
        end
      end
    end
  end
end

@RX14
Copy link
Contributor

RX14 commented Aug 22, 2018

@straight-shoota 10% is fine

@straight-shoota straight-shoota force-pushed the jm/feature/time-refactor-methods branch from b97ce42 to 8a1e06e Compare September 4, 2018 22:06
@bcardiff bcardiff merged commit b89ad33 into crystal-lang:master Apr 5, 2019
@straight-shoota straight-shoota deleted the jm/feature/time-refactor-methods branch April 5, 2019 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants