⚡️ Speed up test suite via caching and fixture scopes to make it ~24% faster#13583
Conversation
|
📝 Docs preview for commit d37b0d5 at: https://74ae3147.fastapitiangolo.pages.dev |
d37b0d5 to
dc196e9
Compare
|
📝 Docs preview for commit dc196e9 at: https://42a11227.fastapitiangolo.pages.dev |
Introduces a security vulnerability into the application. |
There was a problem hiding this comment.
Thanks for the PR @dikos1337! Definitely appreciate you trying to make the test suite more efficient 🙏
With respect to @alv2017's comment, about the security of using @lru_cache for verify_password. I'm also unsure about this change: even if we would consider the usage safe in the context of running our test suite, we also have to consider that these code snippets are used verbatim in the tutorial (e.g. oauth2-scopes), and as such we need to consider whether this is the "best practices" approach we want to show users.
Another important point: by adding lines to the tutorialXXX.py files, the highlighting of specific lines in the tutorial will be wrong. For instance, on the oauth2-scopes link above you can see that e.g. the lines around the get_current_user function are highlighted wrongly (compare with the current version)).
|
@svlandeg I think I found an elegant way to fix this. |
dc196e9 to
8e62206
Compare
|
@dikos1337, which tool were you using to measure the performance of the tests? |
Could you be more specific about which parts of my comments you have addressed? Did you see my comment on line highlighting? |
@svlandeg I don't quite understand what's wrong with highlighting now. I don't make any changes to the documentation anymore. Can you show a screenshot of the problem? |
|
I'm actually having a hard time reviewing the changes because you force pushed changes, meaning that I can't see the original commits anymore, or exactly what changed inbetween versions and inbetween comments. It also looks like your original post is unchanged and doesn't reflect the current state of the PR anymore, and the only comment you gave was that you found a way to fix "this" - but I don't even know what specifically "this" refers to. So let's please take a step back, address the comments on the PR as they were and explain your current changes and their implications? And update the outdated information in the PR? 🙏 E.g. is the speedup still 24%? etc. |
|
@svlandeg The original post is fully consistent with the changes in the current pull request, so I did not change it. "this" refers to both the issues that the use of @lru_cache was visible in the tutorial and the highlighting of specific lines in the tutorial was wrong. In the latest changes I don't change the code in the tutorial, so it's not an issue anymore, the tutorial remains unchanged. I changed the speedup percentage from 31% to 24% in the pull request name because it matches the calculations based on the profiler data, and I also compared the speed of running tests in the current pull request and in other pull requests and it is actually closer to 24% than 31%, but it also depends on the power of the processor, the more powerful the more increase, on my machine the increase is 31%. P.S. no more force pushes from me 🤝 |
svlandeg
left a comment
There was a problem hiding this comment.
These changes look fine to me, but I'll defer to Tiangolo for a final review. Thanks again for the contribution and explanations, @dikos1337 !
This comment was marked as spam.
This comment was marked as spam.
|
@tiangolo Thanks for merging my PR. I wanted to point out that apparently, since the PR was created, mod.engine.dispose() was added to the code. I also added mod.engine.dispose() in this PR, so now it's being called twice. |
|
Oh, dang, I'll check it, thanks! |
While running the tests, I noticed a couple of spots where we could speed things up a bit based on profiling.
Caching
verify_password:verify_passwordhelper (using bcrypt) can be a bit slow, and I saw it was getting called with the same inputs multiple times during the test run.@lru_cache()to it. Since the results are always the same for the same inputs, this is safe.Optimizing Fixture Scopes:
scope="module".Overall Speedup:
The profiling breakdown suggested these specific parts contributed to a relative reduction of ~24.3% of the original time.
Actual measured impact: Running the full test suite (2334 passed, 130 skipped) on my machine took 24.37s before these changes and now takes 16.85s.
That's a time saving of 7.52s, resulting in an overall test suite speedup of ~30.9% (
(24.37 - 16.85) / 24.37 * 100%).Hopefully, this makes the test suite noticeably faster for everyone! Let me know what you think. 🙂