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

Optimize PlayServerInterpreter.toRoutes #3549

Closed
kciesielski opened this issue Feb 29, 2024 · 4 comments · Fixed by #3842
Closed

Optimize PlayServerInterpreter.toRoutes #3549

kciesielski opened this issue Feb 29, 2024 · 4 comments · Fixed by #3842

Comments

@kciesielski
Copy link
Member

The method takes a lot of CPU when running the SimpleGetMultiRoute simulation:

image

This affects throughput dramatically:

image

@kciesielski kciesielski added this to the Performance milestone Feb 29, 2024
@kciesielski kciesielski removed this from the Performance milestone Mar 15, 2024
@dmtran-g
Copy link
Contributor

Hi @kciesielski

I would like to help on improving the SimpleGetMultiRoute scenario for Play.
Any tips / insights on where to start?

I ran perfTests/Gatling/testOnly sttp.tapir.perf.SimpleGetMultiRouteSimulation 3 times with 128 users, and I see numbers similar to the ones shared in this issue description:

  • play.VanillaMulti - mean requests/sec: 12263, 13418, 13269
  • play.TapirMulti - mean requests/sec: 1131, 1190, 1044

Note: the change in #3838 doesn't improve the SimpleGetMultiRoute scenario, the numbers are similar.

@kciesielski
Copy link
Member Author

Hi @dmtran-g, thanks a lot for taking a shot at this! Optimizing that interpreter instantiation in #3838 is definitely worth merging.
Another thing I found today is how the performance test setup is done. The profiler shows that CPU is overwhelmed by calls to PlayServerInterpreter.toRoutes, and it raised my suspicion, because toRoutes seems like a method that's supposed to be called only to initialize the server.
Then, I found this in sttp.tapir.perf.play.Play:

image

This def looks suspicious, so I changed it to lazy val and voila, throughput for SimpleGet reached 160k reqs/sec! I'll do some more tests and profiling, but I'm pretty sure this will be the solution - to acknowledge that router is referenced for each request and make it return a Router that's initialized only once.

@dmtran-g
Copy link
Contributor

This def looks suspicious, so I changed it to lazy val and voila, throughput for SimpleGet reached 160k reqs/sec!

Good catch! That's great news 🎉

@kciesielski
Copy link
Member Author

After some additional tests and profiling I'm pretty convinced that #3842 should be enough to close this issue 🤞

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 a pull request may close this issue.

2 participants