Fix a RecursionError that occurs with deeply-nested request bodies - #1423
Conversation
c3fdd5c to
a664337
Compare
sirosen
left a comment
There was a problem hiding this comment.
Because this is exchanging a very small and simple bit of recursive code for a much larger body of work to "unroll the loop", I think it's appropriate that we make sure we're reshaping this into something which is
- Maximally readable
- Maximally performant
We're reducing readability (net-net), so we should put in the effort to get as much benefit from that trade as possible.
I opened #1424 with a benchmark that I used to validate my suggestions. I encourage its use here -- also feel free to merge or cherry-pick it into here and make further changes! -- to verify these changes:
- use a
deque - use comprehensions rather than iteratively building dicts and lists (even though this requires the addition of a new helper function)
When I measured on Python 3.12 and 3.14, I found rather dramatic improvements, especially in the worst-case type of testing.
I only explore a very small fraction of the possible space with the proposed microbenchmark, so it may be worth our time to enhance it further?
a664337 to
3153849
Compare
Note
This issue was discovered while testing a service release.
The first commit, which introduces a test that demonstrates the problem, takes over 7 seconds locally to eventually throw
RecursionError.The second commit, which eliminates recursion entirely, makes the test succeed in 0.04 seconds.
Fixed
RecursionErrorthat occurs with deeply-nested request bodies.