[WIP, DNM] Track how much time a task spends scheduled. by grynspan · Pull Request #85251 · swiftlang/swift (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation25 Commits17 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
| __attribute__((__cold__)) bool |
|---|
| _swift_task_setTimeSpentRunningTracked(bool isTracked); |
| /// Get the duration spent running the given task (so far) in nanoseconds. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indentation
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.
Comment on lines +523 to +525
| /// 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); |
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?
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.
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!
| std::atomic<bool> AsyncTask::_isTimeSpentRunningTracked { false }; |
| uint64_t AsyncTask::getNanosecondsOnSuspendingClock(void) { |
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) |
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; |
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())) { |
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. :)