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

Handle max_time edge cases for epoch times and add test #24523

Merged
merged 3 commits into from Apr 12, 2016

Conversation

Projects
None yet
5 participants
@BlakeMesdag
Contributor

BlakeMesdag commented Apr 12, 2016

Summary

Fixes the edge cases from #24520 as discussed with @fxn

Other Information

Benchmarks included in #24520 (comment) are approximately the same for this code (within 0.2x).

@rails-bot

This comment has been minimized.

rails-bot commented Apr 12, 2016

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @sgrif (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@fxn

This comment has been minimized.

Member

fxn commented Apr 12, 2016

I had in mind something like this (untested):

def max_mtime(paths)
  time_now  = Time.now
  max_mtime = nil

  paths.each do |path|
    mtime = File.mtime(path)

    # Ignore mtimes in the future.
    next if time_now.compare_without_coercion(mtime) < 0

    if max_mtime.nil? || max_mtime.compare_without_coercion(mtime) < 0
      max_mtime = mtime
    end
  end

  max_mtime
end

There are a number of details here:

  • The implementation is still short and will return the max time for sure (or nil).
  • The next line by itself emphasizes to the reader that we are filtering stuff out.
  • In comparisons we put the (potential) left extreme to the left (receiver) and right extreme to the right, and adjust the comparison with 0 accordingly.
  • Since the method is called max_mtime and it is dealing with mtimes, the variables have "mtime" in their name.

Guess it should run with comparable speed.

@sgrif

This comment has been minimized.

Member

sgrif commented Apr 12, 2016

r? @fxn

@rails-bot rails-bot assigned fxn and unassigned sgrif Apr 12, 2016

@fxn

This comment has been minimized.

Member

fxn commented Apr 12, 2016

By the way, great catch over here (#24520).

Normally there won't be any mtimes in the future, so the original code was going over the whole collection three times:

  1. One loop to compute mtimes and build an array.
  2. Another loop to reject (nothing) and build another array.
  3. A third loop to compute the max.

And this method can be called with hundreds of paths in large apps.

Pheeww!!!

@BlakeMesdag

This comment has been minimized.

Contributor

BlakeMesdag commented Apr 12, 2016

Not much difference in performance, more readable too. Updated:

$ ruby map_reject_vs_memoized.rb 
Calculating -------------------------------------
         double_loop     6.015k i/100ms
     single_memoized     6.703k i/100ms
single_memoized_without_coercion
                        24.494k i/100ms
-------------------------------------------------
         double_loop     74.475k (±11.8%) i/s -    372.930k
     single_memoized     77.028k (±11.5%) i/s -    382.071k
single_memoized_without_coercion
                        469.834k (± 9.0%) i/s -      2.351M

Comparison:
single_memoized_without_coercion:   469834.4 i/s
     single_memoized:    77027.9 i/s - 6.10x slower
         double_loop:    74475.2 i/s - 6.31x slower
@fxn

This comment has been minimized.

Member

fxn commented Apr 12, 2016

Excellent.

Now we can get rid of the first line as well I believe, the method returns nil for empty collections or collections whose mtimes are all in the future.

@BlakeMesdag

This comment has been minimized.

Contributor

BlakeMesdag commented Apr 12, 2016

Awesome, updated and should be good to go now.

@fxn

This comment has been minimized.

Member

fxn commented Apr 12, 2016

Awesome, thanks Blake!

@fxn fxn merged commit b271eda into rails:master Apr 12, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment