From f167d3c0c0904ef36e19fcd37f4f20d33b0e76b6 Mon Sep 17 00:00:00 2001 From: Philippe Proulx Date: Sat, 13 May 2017 13:17:45 -0400 Subject: [PATCH] Collect useless graph's connections MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Before this patch, the connections accumulate in the graph's array of connections and are never destroyed until the graph is destroyed. However, because it is possible to disconnect the ports of a connection, it is possible that a graph contains connections that are useless. This can cause a "leak" of connections, which would be especially experienced with a graph that contains a source which dynamically adds and removes many ports that are connected during its lifetime. The conditions for a connection to be considered useless are: 1. Its reference count is 0. This means only its parent, the graph, controls its existence. 2. Its ports are disconnected. 3. All its created notification iterators are finalized. When all the conditions above are satisfied, it is safe to remove the connection from its parent graph. Conditions 2 and 3 can be checked in: * bt_connection_disconnect_ports(): The connection is dead, detached from its ports, and vice versa, thus satisfying condition 2. * bt_connection_remove_iterator(): The number of notification iterators changes; could drop to 0, thus satisfying condition 3. To accomodate condition 1, a new concept is added to the base object: it is possible to set a "parent is owner" listener which is called when the parent becomes the owner of the object (that is, when the object's reference count drops to 0 and it has a parent). Connection objects use this callback to possibly remove itself from its parent when all three conditions above are satisfied. Signed-off-by: Philippe Proulx Signed-off-by: Jérémie Galarneau --- include/babeltrace/graph/graph-internal.h | 4 ++ include/babeltrace/object-internal.h | 26 ++++++++- lib/graph/connection.c | 68 ++++++++++++++++++++++- lib/graph/graph.c | 9 +++ 4 files changed, 103 insertions(+), 4 deletions(-) diff --git a/include/babeltrace/graph/graph-internal.h b/include/babeltrace/graph/graph-internal.h index 9efd3590..405b8c38 100644 --- a/include/babeltrace/graph/graph-internal.h +++ b/include/babeltrace/graph/graph-internal.h @@ -83,4 +83,8 @@ void bt_graph_notify_ports_disconnected(struct bt_graph *graph, struct bt_port *upstream_port, struct bt_port *downstream_port); +BT_HIDDEN +void bt_graph_remove_connection(struct bt_graph *graph, + struct bt_connection *connection); + #endif /* BABELTRACE_COMPONENT_COMPONENT_GRAPH_INTERNAL_H */ diff --git a/include/babeltrace/object-internal.h b/include/babeltrace/object-internal.h index e130a81e..545a20aa 100644 --- a/include/babeltrace/object-internal.h +++ b/include/babeltrace/object-internal.h @@ -38,8 +38,10 @@ */ struct bt_object { struct bt_ref ref_count; - /* Class-specific release function. */ + /* Class-specific, optional release function. */ bt_object_release_func release; + /* Class-specific, optional "parent is owner" notification listener. */ + bt_object_release_func parent_is_owner_listener; /* @see doc/ref-counting.md */ struct bt_object *parent; }; @@ -66,8 +68,20 @@ static inline void generic_release(struct bt_object *obj) { if (obj->parent) { + void *parent = obj->parent; + + if (obj->parent_is_owner_listener) { + /* + * Object has a chance to destroy itself here + * under certain conditions and notify its + * parent. At this point the parent is + * guaranteed to exist because it's not put yet. + */ + obj->parent_is_owner_listener(obj); + } + /* The release function will be invoked by the parent. */ - bt_put(obj->parent); + bt_put(parent); } else { bt_object_release(obj); } @@ -116,4 +130,12 @@ void bt_object_init(void *ptr, bt_object_release_func release) bt_ref_init(&obj->ref_count, generic_release); } +static inline +void bt_object_set_parent_is_owner_listener(void *obj, + bt_object_release_func cb) +{ + assert(obj); + ((struct bt_object *) obj)->parent_is_owner_listener = cb; +} + #endif /* BABELTRACE_OBJECT_INTERNAL_H */ diff --git a/lib/graph/connection.c b/lib/graph/connection.c index feed5955..7bdbee62 100644 --- a/lib/graph/connection.c +++ b/lib/graph/connection.c @@ -82,6 +82,45 @@ void bt_connection_destroy(struct bt_object *obj) g_free(connection); } +static +void bt_connection_try_remove_from_graph(struct bt_connection *connection) +{ + void *graph = bt_object_borrow_parent(&connection->base); + + if (connection->base.ref_count.count > 0 || + connection->downstream_port || + connection->upstream_port || + connection->iterators->len > 0) { + return; + } + + /* + * At this point we know that: + * + * 1. The connection is dead (ports were disconnected). + * 2. All the notification iterators that this connection + * created, if any, are finalized. + * 3. The connection's reference count is 0, so only the + * parent (graph) owns this connection after this call. + * + * In other words, no other object than the graph knows this + * connection. + * + * It is safe to remove the connection from the graph, therefore + * destroying it. + */ + bt_graph_remove_connection(graph, connection); +} + +static +void bt_connection_parent_is_owner(struct bt_object *obj) +{ + struct bt_connection *connection = container_of(obj, + struct bt_connection, base); + + bt_connection_try_remove_from_graph(connection); +} + struct bt_connection *bt_connection_from_private_connection( struct bt_private_connection *private_connection) { @@ -109,6 +148,8 @@ struct bt_connection *bt_connection_create( } bt_object_init(connection, bt_connection_destroy); + bt_object_set_parent_is_owner_listener(connection, + bt_connection_parent_is_owner); connection->iterators = g_ptr_array_new(); if (!connection->iterators) { BT_PUT(connection); @@ -132,7 +173,8 @@ void bt_connection_disconnect_ports(struct bt_connection *conn) struct bt_component *upstream_comp = NULL; struct bt_port *downstream_port = conn->downstream_port; struct bt_port *upstream_port = conn->upstream_port; - struct bt_graph *graph = (void *) bt_object_get_parent(conn); + struct bt_graph *graph = (void *) bt_object_borrow_parent(conn); + size_t i; if (downstream_port) { downstream_comp = bt_port_get_component(downstream_port); @@ -160,7 +202,28 @@ void bt_connection_disconnect_ports(struct bt_connection *conn) downstream_comp, upstream_port, downstream_port); bt_put(downstream_comp); bt_put(upstream_comp); - bt_put(graph); + + /* + * Because this connection is dead, finalize (cancel) each + * notification iterator created from it. + */ + for (i = 0; i < conn->iterators->len; i++) { + struct bt_notification_iterator *iterator = + g_ptr_array_index(conn->iterators, i); + + bt_notification_iterator_finalize(iterator); + + /* + * Make sure this iterator does not try to remove itself + * from this connection's iterators on destruction + * because this connection won't exist anymore. + */ + bt_notification_iterator_set_connection(iterator, + NULL); + } + + g_ptr_array_set_size(conn->iterators, 0); + bt_connection_try_remove_from_graph(conn); } struct bt_port *bt_connection_get_upstream_port( @@ -282,4 +345,5 @@ void bt_connection_remove_iterator(struct bt_connection *conn, struct bt_notification_iterator *iterator) { g_ptr_array_remove(conn->iterators, iterator); + bt_connection_try_remove_from_graph(conn); } diff --git a/lib/graph/graph.c b/lib/graph/graph.c index ab6b14cf..34e832ad 100644 --- a/lib/graph/graph.c +++ b/lib/graph/graph.c @@ -782,3 +782,12 @@ extern bt_bool bt_graph_is_canceled(struct bt_graph *graph) { return graph ? graph->canceled : BT_FALSE; } + +BT_HIDDEN +void bt_graph_remove_connection(struct bt_graph *graph, + struct bt_connection *connection) +{ + assert(graph); + assert(connection); + g_ptr_array_remove(graph->connections, connection); +} -- 2.34.1