After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 789977 - gegl: Log total processing time for the gegl_processor_work
gegl: Log total processing time for the gegl_processor_work
Status: RESOLVED FIXED
Product: gnome-photos
Classification: Applications
Component: general
3.26.x
Other All
: Normal normal
: ---
Assigned To: GNOME photos maintainer(s)
GNOME photos maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2017-11-06 17:39 UTC by Umang Jain
Modified: 2017-11-27 17:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gegl: Log the total processing time for the GeglProcessor (2.12 KB, patch)
2017-11-24 08:18 UTC, Umang Jain
none Details | Review
gegl: Log the total processing time for the GeglProcessor (2.15 KB, patch)
2017-11-25 07:57 UTC, Umang Jain
committed Details | Review
gegl: Log the total processing time for the GeglProcessor (1.79 KB, patch)
2017-11-27 17:52 UTC, Debarshi Ray
committed Details | Review

Description Umang Jain 2017-11-06 17:39:44 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.
Comment 1 Umang Jain 2017-11-24 08:18:36 UTC
Created attachment 364309 [details] [review]
gegl: Log the total processing time for the GeglProcessor
Comment 2 Debarshi Ray 2017-11-24 18:29:35 UTC
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.
Comment 3 Umang Jain 2017-11-25 07:57:12 UTC
Created attachment 364382 [details] [review]
gegl: Log the total processing time for the GeglProcessor
Comment 4 Debarshi Ray 2017-11-27 17:52:12 UTC
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.
Comment 5 Debarshi Ray 2017-11-27 17:52:57 UTC
Created attachment 364520 [details] [review]
gegl: Log the total processing time for the GeglProcessor

Updated and pushed.