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

Remove try-except overload to check if some attribute exists #159

Closed
wants to merge 4 commits into from

Conversation

garciparedes
Copy link

In the current implementation, there are some try-except blocks to check if an object has some attribute. To reduce the generated overhead by the exception handling logic, I propose to start using the getattr builtin function with a default value instead.

This change doesn't provide a big performance improvement, but it's a constant-factor cost removal with no public API implications.

To check the difference, I build a 1 million node random tree with a height of 25. The logic to build the anynode.AnyNode instances from the tree encoded as raw dictionaries decreased from ~11 seconds to ~8 seconds on my computer (not a big change, but better than nothing 🙂).

@coveralls
Copy link

coveralls commented Mar 17, 2021

Coverage Status

Coverage decreased (-0.0009%) to 99.76% when pulling a0dc60b on garciparedes:master into d63289b on c0fec0de:master.

@garciparedes
Copy link
Author

Hi @c0fec0de! I expect that the coverage failure should not be relevant because it failed because I ended with a negative number of lines added. 😅

@garciparedes
Copy link
Author

Hi @c0fec0de! Any updates about this PR? 🙂

@teauxfu
Copy link
Contributor

teauxfu commented Jun 10, 2021

Nice catch with the attribute name mangling

@c0fec0de
Copy link
Owner

Merged. Thanks for contributing.

@c0fec0de c0fec0de closed this Jun 16, 2021
@c0fec0de
Copy link
Owner

I merged it locally. Will mention you in the release notes.

@garciparedes
Copy link
Author

Hi @c0fec0de! Thanks for accepting muy change. 😉

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

4 participants