GNOME Bugzilla – Bug 627274
cpureport: improve precision of cpu time information
Last modified: 2016-05-31 16:39:39 UTC
Use clock_gettime() for a finer cpu time precission and send 'cpu-times' in nanoseconds to be consistent with 'actual-time'. This way we can get the cpu percent used with 'cpu-time' / 'actual-time' * 100
Created attachment 168207 [details] [review] 0001-cpureport-Improve-precission-of-cpu-time-information.patch
Created attachment 168208 [details] [review] 0001-cpureport-make-use-of-a-monotic-clock-to-measure-act.patch Use a monotonic clock instead of the system time
Created attachment 168209 [details] [review] 0001-cpureport-make-use-of-a-monotic-clock-to-measure-act.patch Delete prints in previous patch
Comment on attachment 168209 [details] [review] 0001-cpureport-make-use-of-a-monotic-clock-to-measure-act.patch Not all systems have a monotonic clock, so it would be better if there was a check done and it would use the monotonic clock if it exists otherwise fallback to the normal way.
Does it makes sense to have to new properties: *wall-clock-type: CLOCK_MONOTONIC or CLOCK_REALTIME *cpu-clock-type: CLOCK_PROCESS_CPUTIME_ID or CLOCK_THREAD_CPUTIME_ID To benchmark encoders, for example, it makes sense to a use the thread cpu time to get values only from the encoding thread, isolating the rest of the tasks like decoding a non-raw source.
Sure sounds good.
Created attachment 168371 [details] [review] 0003-cpureport-Add-cpu-clock-properties.patch
Created attachment 168372 [details] [review] 0004-cpureport-make-wall-time-and-cpu-time-portable.patch
Created attachment 168720 [details] [review] 0001-cpureport-Improve-precission-of-cpu-time-information.patch
Created attachment 168721 [details] [review] 0002-cpureport-make-use-of-a-monotic-clock-to-measure-act.patch
Created attachment 168722 [details] [review] 0003-cpureport-Add-cpu-clock-properties.patch
Created attachment 168723 [details] [review] 0004-cpureport-make-wall-time-and-cpu-time-portable.patch
Review of attachment 168720 [details] [review]: ::: gst/debugutils/cpureport.c @@ +114,2 @@ g_get_current_time (&cur_time); + clock_gettime (CLOCK_PROCESS_CPUTIME_ID, &cur_cpu_time); Please add a configure check for this and conditionally use it. It's only available on POSIX it seems @@ +124,3 @@ + GST_TIMESPEC_TO_TIME (filter->last_cpu_time); + + s = gst_structure_new ("cpu-report", "cpu-time", G_TYPE_INT64, cpu_time, This breaks backward compatibility. Keep cpu-time as double and maybe add a cpu-time-ns as gint64 in nanoseconds
Review of attachment 168721 [details] [review]: ::: gst/debugutils/cpureport.c @@ +114,1 @@ + clock_gettime (CLOCK_MONOTONIC, &cur_time); configure check for this too... and only use it conditionally if available. See core/configure.ac for a configure check for the monotonic clock
I think this is handled in the 4th patch. Can you double check please?
Review of attachment 168722 [details] [review]: ::: gst/debugutils/cpureport.c @@ +42,3 @@ + static const GEnumValue cpu_clock_mode[] = { + {CLOCK_PROCESS_CPUTIME_ID, "Process cpu time", "process"}, + {CLOCK_THREAD_CPUTIME_ID, "Thread cpu time", "thread"}, Are these defined to the same numbers everywhere? Otherwise please add your own constants for them and map them later to the POSIX constants. And only use this conditionally if available. I think these are only available on POSIX too, for everything else just do what was done before.
Review of attachment 168723 [details] [review]: ok, forget what I said about the configure checks before :) Still you should do something about the clock type constants... and all the other comments still apply too, especially the backwards compatible GstStructure
Andoni: do you plan to update these patches?
Closing due to years of inactivity. Please re-open if you/anyone plan to work on the patches, thanks!