Skip to content

Conversation

@gustf
Copy link
Contributor

@gustf gustf commented Nov 21, 2016

After @josevalim commit yesterday I was playing around with this trying to optimize it a bit more.

What I got seems to be around 30% faster testing with a "noop" reduce function like

def noop(_x, acc) do
  acc
end

Here is also the implementation as a gist that I used for testing.

Example result I got was

iex(30)> :timer.tc(ReduceTest, :reduce_new, [1..10000000, 1, &ReduceTest.noop/2]) 
{331031, 1}
iex(31)> :timer.tc(ReduceTest, :reduce_new, [10000000..1, 1, &ReduceTest.noop/2]) 
{339605, 1}
iex(32)> :timer.tc(ReduceTest, :reduce, [1..10000000, 1, &ReduceTest.noop/2])    
{474536, 1}
iex(33)> :timer.tc(ReduceTest, :reduce, [10000000..1, 1, &ReduceTest.noop/2])    
{481838, 1}

I am not sure about the "loop unrolling" on lines 1784-1786 and 1796-1798, as they are a bit of an obfuscation. But they gave 10-15% of the speedup, so I kept them for now.

@josevalim
Copy link
Member

Let's not do the loop unrolling for now. There are many places we could use it, for example, when traversing lists, and we don't in any of those cases.

Copy link
Member

Choose a reason for hiding this comment

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

Can we please match on the ranges just once and do the first <= last check inside the function clause? I want to avoid multiple root level checks (even though the compiler likely optimizes it away anyway).

@lexmag
Copy link
Member

lexmag commented Nov 21, 2016

I think this PR is a good opportunity to replace "faceless" x and y with first and last. :bowtie:

@gustf
Copy link
Contributor Author

gustf commented Nov 21, 2016

Done! :)

@lexmag lexmag merged commit e9a8c63 into elixir-lang:master Nov 21, 2016
@lexmag
Copy link
Member

lexmag commented Nov 21, 2016

Thank you @gustf!

@gustf gustf deleted the reduce-opt-patch branch November 21, 2016 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants