-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[WIP, DNM] Track how much time a task spends scheduled. #85251
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
base: main
Are you sure you want to change the base?
Conversation
|
@swift-ci build toolchain |
|
@swift-ci build toolchain |
|
@swift-ci build toolchain |
|
@swift-ci build toolchain |
| __attribute__((__cold__)) bool | ||
| _swift_task_setTimeSpentRunningTracked(bool isTracked); | ||
|
|
||
| /// Get the duration spent running the given task (so far) in nanoseconds. |
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: indentation
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.
VS Code is not my friend, it seems.
| /// Get the number of nanoseconds spent running this task so far, or `0` if | ||
| /// task duration tracking isn't enabled. | ||
| __attribute__((cold)) uint64_t getTimeSpentRunning(void); |
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.
Should it return a sentinel value if tracking isn't enabled, rather than 0?
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.
No because the caller already does that. We could conceivably return a std::optional but at this time it isn't worth it.
|
@swift-ci build toolchain |
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 assume this is just some experiment not intended to be merged (even after WIP), because that’s not how I think we should be tracking this is we want to make this a real feature — we have tracing.h that we’d utilize for such metrics (including plugging in collection)
|
@ktoso This approach was recommended to me by @mikeash. Let's chat off GitHub and I can give you more context for this experiment. We're not tracing this info for debugging purposes, but rather want to see if we can improve diagnostic output in Swift Testing (where we report how long tests take, but can only do start-of-task to end-of-task without taking into account time spent suspended.) |
|
Gotcha, yeah, this is something we need to fix in general so I was a bit worried about the ad hoc solution. Server systems lacking observability of the runtime is a big issue we need to look into, and it shares much similarities with this need here. Thanks for the context! |
|
@swift-ci build toolchain |
|
@swift-ci build toolchain |
|
|
||
| std::atomic<bool> AsyncTask::_isTimeSpentRunningTracked { false }; | ||
|
|
||
| uint64_t AsyncTask::getNanosecondsOnSuspendingClock(void) { |
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.
This is fundamentally the same as swift_time_now(), will want to reconcile that.
| } | ||
| } | ||
|
|
||
| // MARK: - Time spent running (experimental) |
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.
This all is for the POC only. I wouldn't anticipate exposing it as API (although for Swift Testing to adopt, we may still need some underscored function since it can't directly access the _task property of UnsafeCurrentTask nor Task.)
| TimeSpentRunningStatusRecord() | ||
| : TaskStatusRecord(TaskStatusRecordKind::TimeSpentRunning) {} | ||
|
|
||
| uint64_t TimeSpentRunning = 0; |
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.
My gut tells me we will want to lower the storage for this value into AsyncTask::PrivateStorage for the sake of performance.
| } | ||
|
|
||
| // If we were tracking time spent running, clear that now too. | ||
| if (SWIFT_UNLIKELY(isTimeSpentRunningTracked())) { |
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.
This could theoretically leak if somebody enables tracking before a task starts, then disables it before the task finishes. For this POC, I don't care. :)
|
@swift-ci build toolchain Linux |
|
@swift-ci build toolchain Linux |
|
@swift-ci build toolchain Linux |
|
@swift-ci build toolchain Linux |
|
@swift-ci build toolchain Linux |
|
@swift-ci build toolchain Linux |
…t use malloc/free
|
@swift-ci build toolchain Linux |
No description provided.