GNOME Bugzilla – Bug 789977
gegl: Log total processing time for the gegl_processor_work
Last modified: 2017-11-27 17:53:19 UTC
From #photos: <rishi> uajain: We need to write a patch to track the processing time. <rishi> It's not trivial because we invoke gegl_processor_process in an idle handler. <rishi> We need to sum up the time taken by all those invocations. ... <rishi> Either way, a way to track the processing time will help tracking the performance without having to write toy programs.
Created attachment 364309 [details] [review] gegl: Log the total processing time for the GeglProcessor
Review of attachment 364309 [details] [review]: Thanks for the patch, Umang. This will be a useful addition and looks quite good. Some minor comments: ::: src/photos-gegl.c @@ +346,3 @@ GeglProcessor *processor; + gint64 end; + gint64 *processing_time = NULL; Nitpick: no need to NULL initialize since this scope doesn't own processing_time. The GTask owns it. @@ +354,3 @@ goto done; + processing_time = (gint64 *) g_task_get_task_data (task); Nitpicl: move it below the _get_source_object call, so that the entire "context" of the asynchronous operation is clubbed together. @@ +360,3 @@ + { + end = g_get_monotonic_time (); + *processing_time += end - start; Are we sure that we don't need to add the time taken by the final gegl_processor_work invocation? It will be more readable to move the gegl_processor_work out of the if statement and store its return value, always add the time interval, and then branch on the return value.
Created attachment 364382 [details] [review] gegl: Log the total processing time for the GeglProcessor
Review of attachment 364382 [details] [review]: Thanks for the new patch - looks better. ::: src/photos-gegl.c @@ +359,3 @@ + more_work = gegl_processor_work (processor, NULL); + end = g_get_monotonic_time (); + *processing_time += end - start; The timestamp delta is very likely going to be small enough to fit into a gsize, which can sometimes be smaller than gint64. See below. @@ +388,3 @@ g_task_set_source_tag (task, photos_gegl_processor_process_async); + processing_time = g_malloc (sizeof (gint64)); Sorry for not mentioning this earlier, but we can avoid a malloc/free, and consequently some memory fragmentation, by using the *_TO_POINTER / GPOINTER_TO_ trick. This is especially attractive because most people won't be using the debug output, so the extra malloc/free would be an even more noticeable waste.
Created attachment 364520 [details] [review] gegl: Log the total processing time for the GeglProcessor Updated and pushed.