Implement HttpRequest proxying with __getattr__ for optimized access.#5576
Implement HttpRequest proxying with __getattr__ for optimized access.#5576yotamofek wants to merge 2 commits intoencode:masterfrom
Conversation
carltongibson
left a comment
carltongibson
left a comment
There was a problem hiding this comment.
@yotamofek Looks interesting. Thanks. I need to have a little think about it.
@encode/django-rest-framework-core-team Anyone care to comment?
rpkilby
left a comment
rpkilby
left a comment
There was a problem hiding this comment.
- It would great to have a few simple tests for getattr.
- For whatever reason, CI didn't pick up the PR? Adding tests should hopefully trigger a new build.
|
@rpkilby - hey, I added two simple tests. Do you think it's enough? As for the CI, Travis seems to not like my branch, maybe one of the admins can manually trigger a build? |
|
Somehow travis doesn't seem to know about this PR so I can't trigger a manual build. |
|
@xordoquy I found it under the "Requests" tab, not under the "Pull Requests" tab. |
|
Yup, but still can't do any action on it. |
|
I'll email Travis support, a quick Google search says that's what's needed in these cases. |
| except AttributeError: | ||
| six.reraise(info[0], info[1], info[2].tb_next) | ||
| inner_info = sys.exc_info() | ||
| six.reraise(inner_info[0], inner_info[1], outer_info[2].tb_next) |
There was a problem hiding this comment.
I don't think this is actually necessary. It was in the original implementation, since we were re-raising from the attribute access on the underlying WSGIRequest object. In this case, we're raising from Request.__getattribute__, so catching and reraising should be superfluous.
| wsgi_request = factory.get('/') | ||
|
|
||
| sentinel = object() | ||
| wsgi_request.__dict__['inner_property'] = sentinel |
There was a problem hiding this comment.
It shouldn't be necessary to do reference the underlying `dict. Regular attribute setting should be sufficient.
wsgi_request.inner_property = sentinel
|
Hi @yotamofek - I've left a few comments here, but have implemented them in #5617. I'm going to go ahead and close this in favor of the updated PR. Again, thanks for submitting this. |
4 participants
I was profiling our production server and notices that quite a lot of the time it took for our
ListSerializer.to_representationto run was inRequest.__getattribute__.The way
__getattribute__was implemented was catchingAttributeErrorexceptions, which becomes quite a bottleneck when handling thousands of these exceptions for every request. Catching exceptions is expensive. Fortunately,__getattr__is the interpreter-optimized way of doing the exact same thing. I made sure to keep the same traceback in the case of a trueAttributeError.On our production server, this change sped up certain requests by about 0.4 seconds, which is a large percent of a request that takes ~3 seconds to process.