Add omrthread_get_thread_times() #7491

thallium · 2024-10-10T15:51:16Z

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

Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>

port/common/omrthread.c

tajila · 2024-10-10T15:57:35Z

@babsingh Please review

0xdaryl · 2024-10-10T19:47:51Z

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

thallium · 2024-10-10T19:53:18Z

Fixed spelling

babsingh · 2024-10-11T14:23:32Z

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 · 2024-10-11T14:39:35Z

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

babsingh · 2024-10-11T17:15:09Z

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 · 2024-10-11T19:48:35Z

Moved the function to thrprof.c

Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>

thread/common/thrprof.c

include_core/thread_api.h

Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>

thread/common/thrprof.c

include_core/thread_api.h

thread/common/thrprof.c

babsingh · 2024-10-15T15:29:53Z

@thallium Also, can you add tests for the new function in fvtest/threadextendedtest/processTimeTest.cpp?

thread/common/thrprof.c

thallium · 2024-10-17T19:53:02Z

@babsingh how can I add tests for specific platforms? (since omrthread_get_thread_times will fail on unsupported platforms)

babsingh · 2024-10-17T21:21:10Z

how can I add tests for specific platforms? (since omrthread_get_thread_times will fail on unsupported platforms)

You can use ifdefs:

#if SUPPORTED_PLATFORMS
   Assert(SUCCESS == rc)
   ...
#else
   Assert(FAILURE == rc)
#endif

thallium · 2024-10-18T17:51:08Z

Addressed previous feedbacks and added tests.

thread/common/thrprof.c

fvtest/threadextendedtest/processTimeTest.cpp

thread/common/thrprof.c

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]>

babsingh · 2024-10-22T14:40:05Z

jenkins build all

babsingh · 2024-10-22T16:03:12Z

jenkins build osx,amac,win

Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>

keithc-ca · 2024-10-28T17:17:56Z

thread/common/thrprof.c

+		/* 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;


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.

Can you explain how could dwLowDateTime be negative?

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.

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.

I expect doing as Microsoft advises will result in the same (or even better) code. Let's get this right.

Opened a PR: #7507

Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>

thallium requested review from babsingh, mstoodle and 0xdaryl as code owners October 10, 2024 15:51

thallium added a commit to thallium/openj9 that referenced this pull request Oct 10, 2024

Add JFR CPULoad and ThreadCPU event support

8e202a6

Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>

github-actions bot added comp:core comp:port labels Oct 10, 2024

tajila reviewed Oct 10, 2024

View reviewed changes

port/common/omrthread.c Outdated Show resolved Hide resolved

thallium force-pushed the openj9 branch from 1cd701a to f6726b6 Compare October 10, 2024 19:49

thallium changed the title ~~Add omrthread_get_self_thraed_time()~~ Add omrthread_get_self_thread_time() Oct 10, 2024

tajila reviewed Oct 11, 2024

View reviewed changes

port/common/omrthread.c Outdated Show resolved Hide resolved

thallium force-pushed the openj9 branch 2 times, most recently from 7b3ceba to 52e7090 Compare October 11, 2024 15:33

keithc-ca suggested changes Oct 11, 2024

View reviewed changes

keithc-ca mentioned this pull request Oct 11, 2024

Add JFR CPULoad and ThreadCPU event support eclipse-openj9/openj9#20186

Open

thallium force-pushed the openj9 branch 3 times, most recently from 3777a2a to 6053ad5 Compare October 11, 2024 19:41

thallium added a commit to thallium/openj9 that referenced this pull request Oct 11, 2024

Add JFR CPULoad and ThreadCPU event support

b4ec056

Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>

keithc-ca reviewed Oct 11, 2024

View reviewed changes

thread/common/thrprof.c Outdated Show resolved Hide resolved

keithc-ca reviewed Oct 11, 2024

View reviewed changes

thread/common/thrprof.c Outdated Show resolved Hide resolved

keithc-ca reviewed Oct 11, 2024

View reviewed changes

include_core/thread_api.h Outdated Show resolved Hide resolved

thallium force-pushed the openj9 branch from 6053ad5 to 930d9a6 Compare October 11, 2024 19:55

thallium added a commit to thallium/openj9 that referenced this pull request Oct 11, 2024

Add JFR CPULoad and ThreadCPU event support

5b23512

Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>

keithc-ca suggested changes Oct 11, 2024

View reviewed changes

thread/common/thrprof.c Outdated Show resolved Hide resolved

thread/common/thrprof.c Outdated Show resolved Hide resolved

thallium force-pushed the openj9 branch from 930d9a6 to e42e83b Compare October 11, 2024 20:54

babsingh reviewed Oct 15, 2024

View reviewed changes

include_core/thread_api.h Outdated Show resolved Hide resolved

include_core/thread_api.h Outdated Show resolved Hide resolved

thread/common/thrprof.c Outdated Show resolved Hide resolved

thread/common/thrprof.c Outdated Show resolved Hide resolved

keithc-ca reviewed Oct 15, 2024

View reviewed changes

thread/common/thrprof.c Outdated Show resolved Hide resolved

keithc-ca reviewed Oct 17, 2024

View reviewed changes

thread/common/thrprof.c Outdated Show resolved Hide resolved

thallium force-pushed the openj9 branch from e42e83b to 66d2468 Compare October 18, 2024 15:48

thallium changed the title ~~Add omrthread_get_self_thread_time()~~ Add omrthread_get_thread_times() Oct 18, 2024

babsingh reviewed Oct 18, 2024

View reviewed changes

thread/common/thrprof.c Outdated Show resolved Hide resolved

thallium force-pushed the openj9 branch from 66d2468 to a48756f Compare October 21, 2024 14:30

babsingh reviewed Oct 21, 2024

View reviewed changes

thread/common/thrprof.c Outdated Show resolved Hide resolved

thallium force-pushed the openj9 branch 3 times, most recently from d26b584 to 9e4ea1d Compare October 21, 2024 16:22

babsingh reviewed Oct 21, 2024

View reviewed changes

thread/common/thrprof.c Outdated Show resolved Hide resolved

thread/common/thrprof.c Outdated Show resolved Hide resolved

thallium added 2 commits October 22, 2024 10:28

Add tests for omrthread_get_thread_times

8ac0f94

Signed-off-by: Gengchen Tuo <[email protected]>

thallium force-pushed the openj9 branch from 9e4ea1d to 8ac0f94 Compare October 22, 2024 14:29

babsingh approved these changes Oct 22, 2024

View reviewed changes

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

Add JFR CPULoad and ThreadCPU event support

7a900a4

Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>

thallium added a commit to thallium/openj9 that referenced this pull request Oct 28, 2024

Add JFR CPULoad and ThreadCPU event support

d9a30f1

Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>

keithc-ca reviewed Oct 28, 2024

View reviewed changes

thallium added a commit to thallium/openj9 that referenced this pull request Oct 31, 2024

Add JFR CPULoad and ThreadCPU event support

3da4b50

Depends on: eclipse-omr/omr#7491 Signed-off-by: Gengchen Tuo <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add omrthread_get_thread_times() #7491

Add omrthread_get_thread_times() #7491

thallium commented Oct 10, 2024 •

edited

Loading

tajila commented Oct 10, 2024

0xdaryl commented Oct 10, 2024

thallium commented Oct 10, 2024

babsingh commented Oct 11, 2024 •

edited

Loading

tajila commented Oct 11, 2024

babsingh commented Oct 11, 2024 •

edited

Loading

thallium commented Oct 11, 2024

babsingh commented Oct 15, 2024

thallium commented Oct 17, 2024

babsingh commented Oct 17, 2024

thallium commented Oct 18, 2024

babsingh commented Oct 22, 2024

babsingh commented Oct 22, 2024

keithc-ca Oct 28, 2024

thallium Oct 29, 2024

keithc-ca Oct 29, 2024 •

edited

Loading

thallium Oct 29, 2024

keithc-ca Oct 29, 2024

thallium Oct 29, 2024

Add omrthread_get_thread_times() #7491

Add omrthread_get_thread_times() #7491

Conversation

thallium commented Oct 10, 2024 • edited Loading

tajila commented Oct 10, 2024

0xdaryl commented Oct 10, 2024

thallium commented Oct 10, 2024

babsingh commented Oct 11, 2024 • edited Loading

tajila commented Oct 11, 2024

babsingh commented Oct 11, 2024 • edited Loading

thallium commented Oct 11, 2024

babsingh commented Oct 15, 2024

thallium commented Oct 17, 2024

babsingh commented Oct 17, 2024

thallium commented Oct 18, 2024

babsingh commented Oct 22, 2024

babsingh commented Oct 22, 2024

keithc-ca Oct 28, 2024

Choose a reason for hiding this comment

thallium Oct 29, 2024

Choose a reason for hiding this comment

keithc-ca Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

thallium Oct 29, 2024

Choose a reason for hiding this comment

keithc-ca Oct 29, 2024

Choose a reason for hiding this comment

thallium Oct 29, 2024

Choose a reason for hiding this comment

thallium commented Oct 10, 2024 •

edited

Loading

babsingh commented Oct 11, 2024 •

edited

Loading

babsingh commented Oct 11, 2024 •

edited

Loading

keithc-ca Oct 29, 2024 •

edited

Loading