- Notifications
You must be signed in to change notification settings - Fork6.2k
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Core][aDAG] Remove busy waiting semaphore acquire in linux#47322
base:master
Are you sure you want to change the base?
Conversation
running release testshttps://buildkite.com/ray-project/release/builds/22645 |
| if (*timeout_point <= now) { | ||
| // If the timeout is already passed, try once and return | ||
| got_sem = sem_trywait(sem) == 0; | ||
| } else { |
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 think this can be unified withsem_timedwait:
If the timeout has already expired by the time of the call, and the semaphore could not be locked immediately, then sem_timedwait() fails with a timeout error (errno set to ETIMEDOUT).If the operation can be performed immediately, then sem_timedwait() never fails with a timeout error, regardless of the value of abs_timeout. Furthermore, the validity of abs_timeout is not checked in this case.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.
will fix
| auto secs = std::chrono::duration_cast<std::chrono::seconds>(duration); | ||
| auto ns = std::chrono::duration_cast<std::chrono::nanoseconds>(duration - secs); | ||
| timespec ts; |
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.
nit:
timespec ts;clock_gettime(CLOCK_REALTIME, &ts);// get current timens = ns.count()// Handle the case where nanoseconds overflowauto one_sec_in_ns =1000000000L;ts.tv_sec += secs.count() + ns / one_sec_in_ns;ts.tv_nsec += ns % one_sec_in_ns;
| auto ns = std::chrono::duration_cast<std::chrono::nanoseconds>(duration - secs); | ||
| timespec ts; | ||
| clock_gettime(CLOCK_REALTIME, &ts); // get current time |
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.
can we derivetimespec fromauto now = std::chrono::steady_clock::now();?
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.
+1
Also, should be CLOCK_MONOTONIC rather than CLOCK_REALTIME
| @@ -93,6 +96,38 @@ Status PlasmaObjectHeader::TryToAcquireSemaphore( | |||
| break; | |||
| } | |||
| } while (std::chrono::steady_clock::now() < *timeout_point); | |||
| #else // defined(__Linux__) | |||
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.
Use#elif instead
| @@ -16,6 +16,8 @@ | |||
| #if defined(__APPLE__) || defined(__linux__) | |||
| #include <semaphore.h> | |||
| #include <ctime> | |||
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.
why do we need to includectime? In addition, is it used by both APPLE and LINUX?
| auto ns = std::chrono::duration_cast<std::chrono::nanoseconds>(duration - secs); | ||
| timespec ts; | ||
| clock_gettime(CLOCK_REALTIME, &ts); // get current time |
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.
+1
Also, should be CLOCK_MONOTONIC rather than CLOCK_REALTIME
| #else // defined(__Linux__) | ||
| // Calculate the relative time to wait |
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.
Can you extract this to a private util method?
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.
|
Why are these changes needed?
In linux, we can simply use timedwait without busy-waiting which has performance penalty. I think the implementation complexity is low, and the aDAG should be as performance sensitive as possible, so we should avoid busy waiting if possible
Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.