Bug 33342

Summary: get rid of second layer of callbacks for main loop timeouts/watches
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: Simon McVittie <smcv>
Status: RESOLVED FIXED QA Contact: John (J5) Palmieri <johnp>
Severity: enhancement    
Priority: medium CC: cosimo.alfarano, hp, smcv, thiago, walters
Version: unspecifiedKeywords: patch
Hardware: All   
OS: All   
URL: http://cgit.freedesktop.org/~smcv/dbus/log/?h=simpler-callbacks-33342
Whiteboard: 1.5
i915 platform: i915 features:
Bug Depends on: 33336    
Bug Blocks: 23194, 36164    
Attachments: [1/11] DBusConnection: ref the connection in the timeout handler
[2/11] DBusLoop: remove a layer of pointless abstraction around timeouts
[3/11] DBusLoop: remove second layer of watch callbacks where possible
[4/11] bus-activation: separate the "finished" callback from the watch callback
Remove _dbus_loop_add_watch_full
[6/11] DBusLoop: factor out watch_flags_to_poll_events, watch_flags_from_poll_revents
[7/11] DBusLoop: keep separate lists of watches and timeouts
DBusLoop: move OOM watches to a secondary list instead of flagging them
DBusLoop: inline add_callback, remove_callback into their callers
Drop WatchCallback entirely, just use a list of DBusWatch in the main loop
[11/11] DBusLoop: fold Callback into TimeoutCallback
[5/11] Remove _dbus_loop_add_watch_full
[8/11 v2] DBusLoop: move OOM flag in watches inside the DBusWatch
[9/11 v2] DBusLoop: inline add_callback, remove_callback into their callers
[10/11 v2] Drop WatchCallback entirely, just use a list of DBusWatch in the main loop

Description Simon McVittie 2011-01-21 09:31:50 UTC
At the moment, DBusLoop is given a callback/user-data pair to call when it wants to handle a watch/timeout. In all cases except one, that callback just ignores its user-data and calls dbus_watch_handle() or dbus_timeout_handle().

For timeouts, we can just call dbus_timeout_handle() from _dbus_loop_iterate straight away.

For watches, bus/activation.c has some extra madness in the callback, with a big comment explaining why it shouldn't. Eventually, it just shouldn't, but a good first step would be:

- rename _dbus_add_watch to _dbus_add_watch_full
- add a _dbus_add_watch which just takes a watch, not the closure
- make _dbus_remove_watch just take the watch, not the closure

Then we can kill off _dbus_add_watch_full later.
Comment 1 Simon McVittie 2011-01-24 07:46:05 UTC
I have a branch for this, built on top of the one from Bug #33336; gitweb is in the URL field. Review would be appreciated.

I think this is a job for D-Bus 1.5, really; I'd like to branch 1.4 soon so we can make this sort of change in master.

(In reply to comment #0)
> For watches, bus/activation.c has some extra madness in the callback, with a
> big comment explaining why it shouldn't. Eventually, it just shouldn't

This turned out to be easier than I thought, so I fixed it.
Comment 2 Simon McVittie 2011-01-24 07:51:03 UTC
Created attachment 42374 [details] [review]
[1/11] DBusConnection: ref the connection in the timeout handler

client_timeout_callback in bus/test.c refs the connection across the
timeout invocation, which looks suspiciously like a workaround. If we
make the timeout handler itself ref the connection, we won't need that,
and can simplify timeout handling drastically.

Bug:
Comment 3 Simon McVittie 2011-01-24 07:51:07 UTC
Created attachment 42375 [details] [review]
[2/11] DBusLoop: remove a layer of pointless abstraction around timeouts

Instead of supplying 8 tiny wrapper functions around dbus_timeout_handle,
each with a user_data parameter that's a potentially unsafe borrowed
pointer but isn't actually used, we can call dbus_timeout_handle directly
and save a lot of trouble.

One of the wrappers previously called dbus_timeout_handle repeatedly
if it returned FALSE to indicate OOM, but that timeout's handler never
actually returned FALSE, so there was no practical effect. The rest just
ignore the return, which is documented as OK to do.

Bug:
Comment 4 Simon McVittie 2011-01-24 07:51:10 UTC
Created attachment 42376 [details] [review]
[3/11] DBusLoop: remove second layer of watch callbacks where possible

Similar to the previous commit, almost every use of DBusWatch can just
have the main loop call dbus_watch_handle.

The one exception is the bus activation code; it's had a comment
explaining why it's wrong since 2003. We should fix that one day, but for
now, just migrate it to a new _dbus_loop_add_watch_full which preserves
the second-layer callback.

Bug:
Comment 5 Simon McVittie 2011-01-24 07:51:14 UTC
Created attachment 42377 [details] [review]
[4/11] bus-activation: separate the "finished" callback from the watch callback

This has been marked as broken since 2003...

Bug:
Comment 6 Simon McVittie 2011-01-24 07:51:18 UTC
Created attachment 42378 [details] [review]
Remove _dbus_loop_add_watch_full

Bug:
Comment 7 Simon McVittie 2011-01-24 07:51:22 UTC
Created attachment 42379 [details] [review]
[6/11] DBusLoop: factor out watch_flags_to_poll_events, watch_flags_from_poll_revents

Bug:
Comment 8 Simon McVittie 2011-01-24 07:51:27 UTC
Created attachment 42380 [details] [review]
[7/11] DBusLoop: keep separate lists of watches and timeouts

Bug:
Comment 9 Simon McVittie 2011-01-24 07:51:30 UTC
Created attachment 42381 [details] [review]
DBusLoop: move OOM watches to a secondary list instead of flagging them

This will eventually let us maintain a DBusPollFD[] of just the active
watches, between several iterations. The more immediate benefit is that
WatchCallback can go away, because it only contains a refcount, a
now-useless type, and a watch that already has its own refcount.

Bug:
Comment 10 Simon McVittie 2011-01-24 07:51:35 UTC
Created attachment 42382 [details] [review]
DBusLoop: inline add_callback, remove_callback into their callers

The watch and timeout code paths will diverge completely when we change
WatchCallback * to just be a DBusWatch *, removing the benefit of having
common code.

Bug:
Comment 11 Simon McVittie 2011-01-24 07:51:39 UTC
Created attachment 42383 [details] [review]
Drop WatchCallback entirely, just use a list of DBusWatch in the main loop

Bug:
Comment 12 Simon McVittie 2011-01-24 07:51:43 UTC
Created attachment 42384 [details] [review]
[11/11] DBusLoop: fold Callback into TimeoutCallback

Bug:
Comment 13 Simon McVittie 2011-01-24 07:52:48 UTC
(In reply to comment #12)
> Bug:

Sorry, please ignore these lines; git-bz ate my http://dep.debian.net/deps/dep3/ annotations. :-(
Comment 14 Simon McVittie 2011-01-25 01:58:19 UTC
(In reply to comment #9)
> DBusLoop: move OOM watches to a secondary list instead of flagging them

Unfortunately, this patch isn't right: if a watch is OOM on iteration n, and then it's removed by a different watch/timeout's callback during iteration n+1, it'll still be in the temporary list oom_last_time, so it won't be removed correctly.
Comment 15 Simon McVittie 2011-01-25 05:12:46 UTC
Created attachment 42450 [details] [review]
[5/11] Remove _dbus_loop_add_watch_full

Slightly adjusted to work better with the revised versions of patches 8-11.
Comment 16 Simon McVittie 2011-01-25 05:14:13 UTC
Created attachment 42451 [details] [review]
[8/11 v2] DBusLoop: move OOM flag in watches inside the DBusWatch

This will eventually let us maintain a DBusPollFD[] of just the active
watches, between several iterations. The more immediate benefit is that
WatchCallback can go away, because it only contains a refcount, a
now-useless type, and a watch that already has its own refcount.

This doesn't take any more memory for DBusWatch when not using DBusLoop
(e.g. in client/service code or bindings), because we're just using more
bits in an existing bitfield.

(This fixes Comment #14.)
Comment 17 Simon McVittie 2011-01-25 05:15:04 UTC
Created attachment 42452 [details] [review]
[9/11 v2] DBusLoop: inline add_callback, remove_callback into their callers

Adjusted to apply after 8/11 v2.
Comment 18 Simon McVittie 2011-01-25 05:15:52 UTC
Created attachment 42453 [details] [review]
[10/11 v2] Drop WatchCallback entirely, just use a list of DBusWatch in the main loop

Adjusted to apply after 8/11 v2.
Comment 19 Thiago Macieira 2011-06-13 04:29:05 UTC
Review of attachment 42374 [details] [review]:

Looks good.
Comment 20 Thiago Macieira 2011-06-13 04:50:22 UTC
Review of attachment 42375 [details] [review]:

Looks good.
Comment 21 Thiago Macieira 2011-06-13 04:55:38 UTC
Review of attachment 42376 [details] [review]:

Looks good.
Comment 22 Thiago Macieira 2011-06-13 05:00:07 UTC
Review of attachment 42377 [details] [review]:

Looks good. I guess the next commit removes _dbus_loop_add_watch_full?
Comment 23 Thiago Macieira 2011-06-13 05:00:14 UTC
Review of attachment 42377 [details] [review]:

Looks good. I guess the next commit removes _dbus_loop_add_watch_full?
Comment 24 Thiago Macieira 2011-06-13 05:01:29 UTC
Review of attachment 42450 [details] [review]:

Looks good.
Comment 25 Thiago Macieira 2011-06-13 05:02:50 UTC
Review of attachment 42379 [details] [review]:

Looks good.
Comment 26 Thiago Macieira 2011-06-13 05:03:08 UTC
Review of attachment 42379 [details] [review]:

Looks good.
Comment 27 Thiago Macieira 2011-06-13 05:13:19 UTC
Review of attachment 42380 [details] [review]:

Looks good.
Comment 28 Thiago Macieira 2011-06-13 05:20:01 UTC
Review of attachment 42451 [details] [review]:

Looks ok.
Comment 29 Thiago Macieira 2011-06-13 05:22:36 UTC
Review of attachment 42452 [details] [review]:

Looks good. This is actually cleaner anyway.
Comment 30 Thiago Macieira 2011-06-13 05:41:23 UTC
Review of attachment 42453 [details] [review]:

Looks good.
Comment 31 Thiago Macieira 2011-06-13 05:47:24 UTC
Review of attachment 42384 [details] [review]:

Looks good
Comment 32 Simon McVittie 2011-06-13 09:46:03 UTC
Fixed in git for 1.5.6.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.