Replace additional instances of map{}.flatten with flat_map #14262

Merged
merged 3 commits into from Mar 9, 2014

Conversation

Projects
None yet
4 participants
Contributor

sferik commented Mar 3, 2014

This is a follow-up to #14240. As noted in that issue, using flat_map is significantly faster than map.flatten.

require 'benchmark/ips'

ARRAY = 100.times.map { (0..9).to_a }

def slow
  ARRAY.map { |x| x }.flatten(1)
end

def fast
  ARRAY.flat_map { |x| x }
end

Benchmark.ips do |x|
  x.report('slow') { slow }
  x.report('fast') { fast }
end
slow 12348.2 (±9.0%) i/s -  61450 in 5.017159s
fast 56647.8 (±7.2%) i/s - 282152 in 5.006973s
Contributor

sferik commented Mar 4, 2014

Okay, this should be ready to merge now.

Member

arthurnn commented Mar 8, 2014

THIS LGTM .. .nice catch @sferik

Owner

spastorino commented Mar 9, 2014

flat_map is not equivalent to map.flatten, it is only equivalent to map.flatten(1).
Unsure if all of those map.flatten calls would be equal. @sferik is that what you're saying?.

Contributor

sferik commented Mar 9, 2014

@spastorino I understand the difference between map.flatten and flat_map. I’ve looked at each of these cases individually and think they are safe to replace but please don’t take my word for it. I hope this patch will be reviewed by others who are more familiar with the code than I am. Obviously, I made sure all the tests pass but there may not be tests for more than one level of flattening when it may be necessary. If there are cases where it is not safe to replace map.flatten with flat_map, we should add tests for that.

Owner

pixeltrix commented Mar 9, 2014

In terms of the routing code the NFA and GTG related ones are for the visualizer part of Journey which isn't used in a Rails application so any performance gain is non-existent. In fact I'm tempted to move the code out into a separate gem so that we don't get PRs for it in the main Rails repo.

The other routing one is the one that particularly concerns me since you can nest optional segments in routes more than one deep - I'll test it to see if it's affected.

As for the others, they all seem fine.

Owner

spastorino commented Mar 9, 2014

@sferik 👍 I guessed that you knew it and you were just trying to get rid of it.

Owner

pixeltrix commented Mar 9, 2014

@sferik the flat_map change in optional_names doesn't change the behaviour but I think the method could be be better implemented using a custom visitor class. So I'm happy to merge it and look at refactoring it for 4.2

pixeltrix merged commit ec23277 into rails:master Mar 9, 2014

1 check passed

default The Travis CI build passed
Details

sferik changed the title from Replace additional instances of map.flatten with flat_map to Replace additional instances of map{}.flatten with flat_map May 19, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment