Skip to content
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

Merged
merged 2 commits into from
Oct 22, 2024
Merged

Conversation

thallium
Copy link
Contributor

@thallium thallium commented Oct 10, 2024

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

thallium added a commit to thallium/openj9 that referenced this pull request Oct 10, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
port/common/omrthread.c Outdated Show resolved Hide resolved
@tajila
Copy link
Contributor

tajila commented Oct 10, 2024

@babsingh Please review

@0xdaryl
Copy link
Contributor

0xdaryl commented Oct 10, 2024

Minor nit: please fix the spelling of "thread" in your commit and PR titles. Thanks.

@thallium thallium changed the title Add omrthread_get_self_thraed_time() Add omrthread_get_self_thread_time() Oct 10, 2024
@thallium
Copy link
Contributor Author

Fixed spelling

@babsingh
Copy link
Contributor

babsingh commented Oct 11, 2024

The new function needs to be added similar to the existing functions in thrprof.c: example.

In the new function, the output of omrthread_get_cpu_time and omrthread_get_user_time is aggregated and returned. thrprof.c should have basic functionalities. The aggregation should be done in the upstream project. In other words, the new function shouldn't reside in OMR.

omrthread_get_user_time doesn't have support for Linux. Does anything prevent us to add Linux support in omrthread_get_user_time?

@tajila
Copy link
Contributor

tajila commented Oct 11, 2024

omrthread_get_user_time doesn't have support for Linux. Does anything prevent us to add Linux support in omrthread_get_user_time?

AFAIK, the only way to get user time reliably on linux is with getrusage(RUSAGE_THREAD, ... which can only be called on the current thread. This is why I proposed making a new API since the existing one takes an arbritrary thread as a param.

The goal is to do something like omrthread_get_process_times which returns omrthread_process_time_t

port/common/omrthread.c Outdated Show resolved Hide resolved
@thallium thallium force-pushed the openj9 branch 2 times, most recently from 7b3ceba to 52e7090 Compare October 11, 2024 15:33
port/common/omrthread.c Outdated Show resolved Hide resolved
port/common/omrthread.c Outdated Show resolved Hide resolved
port/common/omrthread.c Outdated Show resolved Hide resolved
port/common/omrthread.c Outdated Show resolved Hide resolved
port/common/omrthread.c Outdated Show resolved Hide resolved
@babsingh
Copy link
Contributor

babsingh commented Oct 11, 2024

The new function needs to be added similar to the existing functions in thrprof.c: example.

@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 thrprof.c:omrthread_get_process_times and follow its implementation. the correct usage of the new function should require #include "omrthread.h" or #include "thread_api.h".

Also, include the justification in the commit on why the new function is needed.

@thallium
Copy link
Contributor Author

Moved the function to thrprof.c

thallium added a commit to thallium/openj9 that referenced this pull request Oct 11, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
thread/common/thrprof.c Outdated Show resolved Hide resolved
thread/common/thrprof.c Outdated Show resolved Hide resolved
thallium added a commit to thallium/openj9 that referenced this pull request Oct 11, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
thread/common/thrprof.c Outdated Show resolved Hide resolved
thread/common/thrprof.c Outdated Show resolved Hide resolved
@thallium thallium force-pushed the openj9 branch 3 times, most recently from d26b584 to 9e4ea1d Compare October 21, 2024 16:22
thread/common/thrprof.c Outdated Show resolved Hide resolved
thread/common/thrprof.c Outdated Show resolved Hide resolved
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 <gengchen.tuo@ibm.com>
Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
@babsingh
Copy link
Contributor

jenkins build all

@babsingh
Copy link
Contributor

jenkins build osx,amac,win

@babsingh babsingh merged commit e3e7f4a into eclipse-omr:master Oct 22, 2024
17 checks passed
thallium added a commit to thallium/openj9 that referenced this pull request Oct 22, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
thallium added a commit to thallium/openj9 that referenced this pull request Oct 28, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
Comment on lines +1063 to +1065
/* 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;
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

@keithc-ca keithc-ca Oct 29, 2024

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened a PR: #7507

thallium added a commit to thallium/openj9 that referenced this pull request Oct 31, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
thallium added a commit to thallium/openj9 that referenced this pull request Nov 4, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
thallium added a commit to thallium/openj9 that referenced this pull request Nov 7, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
tajila pushed a commit to tajila/openj9 that referenced this pull request Nov 7, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
tajila pushed a commit to tajila/openj9 that referenced this pull request Nov 7, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
tajila pushed a commit to tajila/openj9 that referenced this pull request Nov 7, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
tajila pushed a commit to tajila/openj9 that referenced this pull request Nov 7, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
thallium added a commit to thallium/openj9 that referenced this pull request Nov 7, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
thallium added a commit to thallium/omr that referenced this pull request Nov 7, 2024
This is missed in eclipse-omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
thallium added a commit to thallium/openj9 that referenced this pull request Nov 11, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
thallium added a commit to thallium/openj9 that referenced this pull request Nov 12, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
tajila pushed a commit to tajila/openj9 that referenced this pull request Nov 12, 2024
Depends on: eclipse-omr/omr#7491

Signed-off-by: Gengchen Tuo <gengchen.tuo@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants