-
Notifications
You must be signed in to change notification settings - Fork 396
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
Add omrthread_get_thread_times() #7491
Conversation
Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>
@babsingh Please review |
Minor nit: please fix the spelling of "thread" in your commit and PR titles. Thanks. |
Fixed spelling |
The new function needs to be added similar to the existing functions in In the new function, the output of
|
AFAIK, the only way to get user time reliably on linux is with The goal is to do something like |
7b3ceba
to
52e7090
Compare
@thallium port and thread libraries are standalone. a thread library function shouldn't be added to the port library. see an existing function such as Also, include the justification in the commit on why the new function is needed. |
3777a2a
to
6053ad5
Compare
Moved the function to |
Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>
Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>
@thallium Also, can you add tests for the new function in |
@babsingh how can I add tests for specific platforms? (since |
You can use
|
Addressed previous feedbacks and added tests. |
d26b584
to
9e4ea1d
Compare
This function returns the user and system cpu time of the calling thread. This new function is needed so we can get both user time and system time using only one system call. Related: eclipse-openj9/openj9#20186 Signed-off-by: Gengchen Tuo <[email protected]>
Signed-off-by: Gengchen Tuo <[email protected]>
jenkins build all |
jenkins build osx,amac,win |
Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>
Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>
/* Time is in 100's of nanos. Convert to nanos. */ | ||
threadTime->sysTime = ((int64_t)kernelTime.dwLowDateTime | ((int64_t)kernelTime.dwHighDateTime << 32)) * 100; | ||
threadTime->userTime = ((int64_t)userTime.dwLowDateTime | ((int64_t)userTime.dwHighDateTime << 32)) * 100; |
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.
When dwLowDateTime
is negative, this yields the wrong result (it ignores dwHighDateTime
).
See advice at https://learn.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-filetime and example code in gtest.cc
.
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 explain how could dwLowDateTime
be negative?
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.
Ok, dwLowDateTime
uses the type DWORD
which is equivalent to uint32_t
(so it can't be negative). Either we need an extra cast to uint64_t
before casting to int64_t
(casts should not change both the width and signedness), or (preferred) we should follow the advice from Microsoft to use ULARGE_INTEGER
and its QuadPart
member like the example in gtest.cc
I referenced.
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 Microsoft's advice is more about manipulating the time value so using ULARGE_INTEGER
just to get the time value seems unnecessary to me. I'll open a PR to fix all the related casting.
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 expect doing as Microsoft advises will result in the same (or even better) code. Let's get this right.
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.
Opened a PR: #7507
Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>
This function returns the user and system cpu time of the calling thread.
This new function is needed so we can get both user time and
system time using only one system call.
Related: eclipse-openj9/openj9#20186