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

[Core][Streaming Generator] Potential perf regression from serve streaming handle fix (https://github.com/ray-project/ray/pull/37972) #38163

Closed
rkooo567 opened this issue Aug 7, 2023 · 1 comment · Fixed by #38280
Assignees
Labels
bug Something that is supposed to be working; but isn't core Issues that should be addressed in Ray Core P0 Issues that should be fixed in short order

Comments

@rkooo567
Copy link
Contributor

rkooo567 commented Aug 7, 2023

What happened + What you expected to happen

It is expected #37972 causes some level of perf issue. But it seems like this actually impacts Aviary. @Yard1 will double verify this Monday if this is true.

To fix the issue, we should

  1. post the cpp level code to a thread pool. Previously, we called cpp code that doesn't hold GIL inside a main thread, so event loop thread could focus on running python code. Now, we are running the cpp code inside the event loop, so nogil doesn't have much impact anymore (because there's no concurrency).
  2. Start running microbenchmark that could catch regressions. (@edoakes is it also possible to start tracking serve microbenchmark separately? )

Versions / Dependencies

master

Reproduction script

n/a

Issue Severity

None

@rkooo567 rkooo567 added bug Something that is supposed to be working; but isn't P0 Issues that should be fixed in short order core Issues that should be addressed in Ray Core labels Aug 7, 2023
@rkooo567 rkooo567 self-assigned this Aug 7, 2023
@rkooo567 rkooo567 added Ray 2.7 and removed Ray 2.7 labels Aug 7, 2023
@Yard1
Copy link
Member

Yard1 commented Aug 7, 2023

I can confirm Ray 178ae0f is the issue, running the benchmark with a commit right before it gives the expected performance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that is supposed to be working; but isn't core Issues that should be addressed in Ray Core P0 Issues that should be fixed in short order
Projects
None yet
2 participants