-
Notifications
You must be signed in to change notification settings - Fork 164
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
Sleep while polling sockets on Windows #5594
Conversation
src/realm/util/network.cpp
Outdated
@@ -1155,6 +1155,8 @@ bool Service::IoReactor::wait_and_advance(clock::time_point timeout, clock::time | |||
ret++; | |||
} | |||
|
|||
std::this_thread::sleep_for(std::chrono::milliseconds(10)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, i think maybe this should be on line 1151 like
if (m_pollfd_slots.size() > 1) {
...
} else if (m_wakeup_pipe.is_signaled()) {
...
} else {
sleep()
}
Maybe what's going on here is that you don't have any active sync sessions and so you're stuck busy waiting for a sync session to open or the Service to get something posted to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opted to preserve the old behavior (no else
for if (m_wakeup_pipe.is_signaled())
) because I don't know enough about networking on Windows and in Core to judge whether it was intentional that we always check for the wakeup pipe, regardless of whether we polled the sockets. Happy to make the suggested change if you believe the original code was wrong though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking as red until we had a chance to discuss it offline
The failing tests seem to be due to timeouts starting stitch and on macos/ubuntu, so they should not be caused by the changes in this PR. |
We had the discussion. Fee free to merge it. |
I propose an alternative fix here #5599 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we use the alternative fix unless something critical is found with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After profiling both fixes and sharing the results I will leave it to Core team to decide which fix to go in.
I'm going to merge this as is to make our Dart tests reliably green. We can always improve the situation with #5599 whenever that passes review. |
What, How & Why?
This appears to address the issue reported in #5591.
I have no idea if that's a real fix or making things worse, but when testing locally, it made CPU usage go from 100% on both cores to ~10-20%.
☑️ ToDos