Skip to content

Conversation

@canni
Copy link
Contributor

@canni canni commented Jun 4, 2015

IMO this reads nicer, and it's easier to understand :)

@eapache
Copy link
Owner

eapache commented Jun 4, 2015

I don't disagree stylistically, but these changes do add about 5% to the benchmark times on my machine, which isn't ideal.

@canni
Copy link
Contributor Author

canni commented Jun 4, 2015

That is strange, but looks like it's second commit causes slowdown, first actually gives ~5% speedup
Try again with only first of two commits

@eapache
Copy link
Owner

eapache commented Jun 6, 2015

Yup, first commit on its own seems to give a (marginal) speed-up. I think that makes sense; len(foo) looks like a function call but I believe it reduces to just a variable access, so storing it in a variable explicitly just makes more work for the compiler.

Push the branch back to just the first commit and I'll merge this.

@canni
Copy link
Contributor Author

canni commented Jun 6, 2015

@eapache done

eapache added a commit that referenced this pull request Jun 6, 2015
Small arithmetic simplification
@eapache eapache merged commit ded5959 into eapache:master Jun 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants