Skip to content

Use getattr for better performance #4011

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

Closed
wants to merge 1 commit into from

Conversation

chrisseto
Copy link

Description

__getattribute__ is called every time any attribute is accessed on request. It catches AttributeErrors in order to proxy to the underlying request.
__getattr__ is called, by python, whenever an attribute is missing and has a much lower overhead.

@kevin-brown
Copy link
Member

This behaviour was changed to the current set up in #2530 because of an issue mentioned in #2108.

You can see that this change actually breaks a test that we specifically put in, because using __getattr__ hides the source of any nested AttributeError that happens to be raised.

@chrisseto
Copy link
Author

@kevin-brown, Thanks for pointing that out!

I'll see if I can fix that test, there may be a terrible way of getting a good/proper traceback.

If you don't think this is worth pursuing please let me know but we are seeing about a 1/10th of a second speed increase on our end.

  __getattribute__ is called everytime any attribute is accessed on
  request. It catches AttributeErrors in order to proxy to the
  underlying request. getattr is called, by python, whenever an
  attribute is missing and has a much lower overhead.
@xordoquy
Copy link
Collaborator

I'm thinking we may introduce an optional "performance mode" which would trigger a couple of optimizations as trade off for more comprehensive stack traces.

Thoughts ?

@chrisseto
Copy link
Author

@xordoquy For this specific PR I was able to get the performance without sacrificing the legible stack traces.
That being I like to idea of a "performance mode" but it may apply oddly to cases such as this one.

@tomchristie
Copy link
Member

I don't think we're likely to accept speculative "performance" changes, without real-world demonstrates of how they effect things at a request-response level.

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.

4 participants