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 711857 - Avoid needless event copies when queueing from a backend to a stage
Avoid needless event copies when queueing from a backend to a stage
Status: RESOLVED FIXED
Product: clutter
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: clutter-maint
clutter-maint
Depends on:
Blocks:
 
 
Reported: 2013-11-11 18:19 UTC by Rui Matos
Modified: 2014-03-15 19:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Avoid needless event copies when queueing from a backend to a stage (7.99 KB, patch)
2013-11-11 18:19 UTC, Rui Matos
none Details | Review
Avoid needless event copies when queueing from a backend to a stage (8.53 KB, patch)
2013-11-21 14:06 UTC, Rui Matos
needs-work Details | Review
Avoid needless event copies when queueing from a backend to a stage (8.22 KB, patch)
2013-11-21 17:20 UTC, Rui Matos
committed Details | Review

Description Rui Matos 2013-11-11 18:19:30 UTC
See patch. FWIW, it seems to work fine for gnome-shell/mutter on the
x11 backend.
Comment 1 Rui Matos 2013-11-11 18:19:32 UTC
Created attachment 259578 [details] [review]
Avoid needless event copies when queueing from a backend to a stage

All backends follow the same pattern of queueing events first in
ClutterMainContext, then copying them to a ClutterStage queue and
immediately free them. Instead, we can just pass ownership of events
directly to ClutterStage thus avoiding the allocation and copy in
between.
Comment 2 Rui Matos 2013-11-21 14:06:49 UTC
Created attachment 260440 [details] [review]
Avoid needless event copies when queueing from a backend to a stage

--

I forgot an #include "clutter-stage-private.h" in a few files.
Comment 3 Emmanuele Bassi (:ebassi) 2013-11-21 14:41:57 UTC
Review of attachment 260440 [details] [review]:

::: clutter/clutter-stage-private.h
@@ +62,3 @@
 gboolean            _clutter_stage_do_update             (ClutterStage          *stage);
 
+void     _clutter_take_event                              (ClutterEvent *event);

the name is not really nice. at most, it should be clutter_event_take(), and be declared in clutter-event-private.h.

to be fair, though, I don't see the point of this churn: creating a wrapper around clutter_stage_queue_event() just to take the stage out of the event? all places that have access to _clutter_stage_queue_event() also have access to the stage to queue on, or they can simply get the event's stage if they don't.

I'd like to move away from the global "push an event" API, and make it clear that the events are queued on a per-stage basis.

::: clutter/clutter-stage.c
@@ -951,3 +951,3 @@
   first_event = priv->event_queue->length == 0;
 
-  g_queue_push_tail (priv->event_queue, clutter_event_copy (event));
+  g_queue_push_tail (priv->event_queue, event);

I'd rather add a boolean 'copy_event' argument to clutter_stage_queue_event(), and conditionally copy the event.
Comment 4 Rui Matos 2013-11-21 17:20:25 UTC
Created attachment 260470 [details] [review]
Avoid needless event copies when queueing from a backend to a stage

--

Ok, this works as well for me, although I generally don't like boolean
arguments. But this is only internal API and there's precedent in
_clutter_event_push() .
Comment 5 Rui Matos 2014-03-05 14:48:19 UTC
I'd really like to have something like this at least in the evdev backend. As a compositor, we see all events coming in so reducing the amount of alloc/copy/free on this path should help to at least reduce memory fragmentation.
Comment 6 Emmanuele Bassi (:ebassi) 2014-03-15 19:43:01 UTC
Review of attachment 260470 [details] [review]:

okay, looks good to me.
Comment 7 Emmanuele Bassi (:ebassi) 2014-03-15 19:46:29 UTC
Attachment 260470 [details] pushed as e70a010 - Avoid needless event copies when queueing from a backend to a stage