Skip to content
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

Performance improvements #11

Merged
merged 15 commits into from
Dec 9, 2018
Merged

Conversation

samipfjo
Copy link
Contributor

@samipfjo samipfjo commented Dec 1, 2018

This PR contains a slew of performance improvements, both small and large.

The largest improvement included is the replacement of deepcopy() with a simple list comprehension, which dramatically speeds up leaf generation.

Other changes include:

  • Moving stdout handling to a separate thread (the overhead for printing that frequently turned out to be bananas)

  • Replacing a couple of the manual implementations of random number generation with faster built-ins (I ran a bunch of rounds of timeit to make sure they were improvements).

  • Addition of .idea/ to the .gitignore (the folder that PyCharm uses to store its project config)

  • Some general format cleanup; adding whitespace between code blocks and such. Not a meticulous overhaul by any means.

Luke Jones added 12 commits November 28, 2018 13:37
- rand_in_range(0, 1) is equivalent to random.random()

- random.uniform(-1, 1) performs much better than random.choice([-1, 1])
* random.random()
After random()-related changes it became a wrapper for
random.uniform(-1, 1)
There are usually fewer stems than leaves, so it deserves a lower
refresh rate
Mostly caching variables that are used repeatedly and reducing
performance penalties due to scoping
@samipfjo
Copy link
Contributor Author

samipfjo commented Dec 1, 2018

Quaking Aspen with leaves (seed 100):

Before:
Stems made: 1131 in 2.632385 seconds
Made 31002 leaves and 0 blossoms in 5.648526 seconds
Tree generated in 8.346872 seconds

After:
Stems made: 1188 in 2.405932 seconds
Made 30391 leaves and 0 blossoms in 2.056793 seconds
Tree generated in 4.538678 seconds


Douglas Fir with leaves (seed 100):

Before:
Stems made: 5782 in 24.919251 seconds
Made 354022 leaves and 0 blossoms in 55.897657 seconds
Tree generated in 81.486497 seconds

After:
Stems made: 6151 in 19.982167 seconds
Made 351227 leaves and 0 blossoms in 15.782944 seconds
Tree generated in 36.447690 seconds


Weeping Willow with leaves (seed 100):

Before:
Stems made: 30326 in 94.704319 seconds
Made 388967 leaves and 0 blossoms in 65.510431 seconds
Tree generated in 161.144178 seconds

After:
Stems made: 39770 in 124.523556 seconds
Made 483994 leaves and 0 blossoms in 24.791790 seconds
Tree generated in 150.350708 seconds


Summary

Stems:
There appears to be a performance increase in stem generation in the Douglas Fir comparison, and a slight performance increase in the Quaking Aspen comparison. Running the numbers on the Weeping Willow comparison, however, results in an inconclusive race.

Leaves:
Leaf generation is unquestionably faster. I'm so glad that deepcopy() isn't needed anymore.


Testbench specs:

Acer Aspire v3-771G (laptop)
2.4GHz Quad-Core i7-3630QM | 12GB DDR3 1600 MHz | SATA3 SSD

@samipfjo
Copy link
Contributor Author

samipfjo commented Dec 1, 2018

It would appear that I've messed up seeding. Hold off on merging for now.

I've figured it out; seeds are still working. By replacing rand_for_param_var() with random.uniform(-1, 1) I changed the numbers that are pumped out via a given seed. They are still consistent in the new system, they just don't sync up with the old ones.

I did notice a different bug while reading through the diff; got that patched up.

Luke Jones added 3 commits November 30, 2018 23:38
mathutils.Vector is implemented in C. By extending the class in Python, a bunch of Python-related overhead is added, which damages the speed gains from the original C implementation. This change brings Quaking Aspen seed 100 from an average of 4.317s down to an average of 3.981s
@samipfjo
Copy link
Contributor Author

samipfjo commented Dec 4, 2018

Tried another change. Further testing showed little difference in performance, hence the revert.

@friggog
Copy link
Owner

friggog commented Dec 6, 2018

I'll try and get around to fully reviewing and testing this on the weekend as there are a lot of changes to merge without any checks! Nonetheless, very impressive work!

@friggog friggog merged commit 4cb4610 into friggog:master Dec 9, 2018
@samipfjo samipfjo deleted the performance-improvements branch December 9, 2018 22:51
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.

None yet

2 participants