Skip to content

Small arithmetic simplification#5

Merged
eapache merged 1 commit intoeapache:masterfrom
canni:master
Jun 6, 2015
Merged

Small arithmetic simplification#5
eapache merged 1 commit intoeapache:masterfrom
canni:master

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants