Conversation
Summary of ChangesHello @oI0ck, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the system's timekeeping capabilities by integrating support for CLOCK_THREAD_CPUTIME_ID. This allows applications to accurately measure the CPU time consumed by individual threads, aligning with POSIX standards and providing more granular performance monitoring. The change involves updating header definitions and modifying the core clock_gettime implementation to leverage new thread information retrieval mechanisms. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for CLOCK_THREAD_CPUTIME_ID to clock_gettime. The implementation is a good start, but there are a few important issues to address. The signature of threadsinfo() has been modified, which constitutes a breaking API change that contradicts the PR description. Additionally, in the new logic for clock_gettime, there's an unnecessary function call and missing error handling, which could lead to incorrect behavior. I've provided suggestions to fix these issues and improve the code's robustness and maintainability.
|
|
||
|
|
||
| extern int threadsinfo(int n, threadinfo_t *info); | ||
| extern int threadsinfo(int *tid, unsigned int flags, int n, threadinfo_t *info); |
There was a problem hiding this comment.
The signature of the extern function threadsinfo has been changed. This is a breaking API change, as any existing code calling this function will fail to compile. The pull request description marks this as a "non-breaking change", which is inaccurate. Please update the PR description to reflect that this is a breaking change.
| gettime(&now, &offs); | ||
|
|
||
| if (clk_id == CLOCK_REALTIME) | ||
| if (clk_id == CLOCK_REALTIME) { | ||
| now += offs; | ||
| } | ||
| else if (clk_id == CLOCK_THREAD_CPUTIME_ID) { | ||
| tid = gettid(); | ||
| threadsinfo(&tid, PH_THREADINFO_CPUTIME, 1, &info); | ||
| now = info.cpuTime; | ||
| } |
There was a problem hiding this comment.
There are a couple of issues in this block:
- The
gettime()function is called unconditionally, even forCLOCK_THREAD_CPUTIME_IDwhere its results are not used. This is inefficient. The call should only be made when needed. - The return value of
threadsinfo()is not checked. If it fails,info.cpuTimecould contain an uninitialized value, leading toclock_gettime()returning an incorrect time with a success status (EOK). The return value should be checked for errors.
Here's a suggested refactoring that addresses both points:
if (clk_id == CLOCK_THREAD_CPUTIME_ID) {
tid = gettid();
if (threadsinfo(&tid, PH_THREADINFO_CPUTIME, 1, &info) < 0) {
return -1; /* Assumes threadsinfo sets errno on failure */
}
now = info.cpuTime;
}
else {
gettime(&now, &offs);
if (clk_id == CLOCK_REALTIME) {
now += offs;
}
}| if (clk_id != CLOCK_REALTIME && clk_id != CLOCK_MONOTONIC && clk_id != CLOCK_MONOTONIC_RAW && clk_id != CLOCK_THREAD_CPUTIME_ID) { | ||
| errno = EINVAL; | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
This chain of && conditions to validate clk_id is becoming long and can be hard to read. Using a switch statement would be more readable and maintainable, especially if more clock IDs are added in the future.
switch (clk_id) {
case CLOCK_REALTIME:
case CLOCK_MONOTONIC:
case CLOCK_MONOTONIC_RAW:
case CLOCK_THREAD_CPUTIME_ID:
break;
default:
errno = EINVAL;
return -1;
}1 participant
Description
This pull request adds support for POSIX specified
CLOCK_THREAD_CPUTIME_IDflag inclock_gettimeTypes of changes
How Has This Been Tested?
Checklist:
Special treatment