From e6a8e8e4744633807083a077ff9f101eb97d9801 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?= Date: Tue, 19 Jan 2016 13:40:30 -0500 Subject: [PATCH] Implement new CTF-IR reference counting scheme MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit See doc/ref-counting.md for the rationale and description of this architecture change. Signed-off-by: Jérémie Galarneau --- formats/ctf/ir/event.c | 140 ++++++------------ formats/ctf/ir/stream-class.c | 74 +++------ formats/ctf/ir/stream.c | 61 +++++--- formats/ctf/ir/trace.c | 25 +--- formats/ctf/writer/writer.c | 4 +- include/babeltrace/ctf-ir/event-internal.h | 15 -- .../babeltrace/ctf-ir/stream-class-internal.h | 6 - include/babeltrace/ctf-ir/stream-internal.h | 2 - tests/lib/test_ctf_writer.c | 3 - 9 files changed, 110 insertions(+), 220 deletions(-) diff --git a/formats/ctf/ir/event.c b/formats/ctf/ir/event.c index 226251f7..a36779de 100644 --- a/formats/ctf/ir/event.c +++ b/formats/ctf/ir/event.c @@ -167,13 +167,16 @@ int bt_ctf_event_class_set_id(struct bt_ctf_event_class *event_class, { int ret = 0; struct bt_value *obj = NULL; + struct bt_ctf_stream_class *stream_class = NULL; + if (!event_class) { ret = -1; goto end; } - if (event_class->stream_class) { + stream_class = bt_ctf_event_class_get_stream_class(event_class); + if (stream_class) { /* * We don't allow changing the id if the event class has already * been added to a stream class. @@ -195,6 +198,7 @@ int bt_ctf_event_class_set_id(struct bt_ctf_event_class *event_class, end: BT_PUT(obj); + BT_PUT(stream_class); return ret; } @@ -320,16 +324,7 @@ end: struct bt_ctf_stream_class *bt_ctf_event_class_get_stream_class( struct bt_ctf_event_class *event_class) { - struct bt_ctf_stream_class *stream_class = NULL; - - if (!event_class) { - goto end; - } - - stream_class = event_class->stream_class; - bt_get(stream_class); -end: - return stream_class; + return (struct bt_ctf_stream_class *) bt_object_get_parent(event_class); } struct bt_ctf_field_type *bt_ctf_event_class_get_payload_type( @@ -537,33 +532,39 @@ end: struct bt_ctf_event *bt_ctf_event_create(struct bt_ctf_event_class *event_class) { struct bt_ctf_event *event = NULL; + struct bt_ctf_stream_class *stream_class = NULL; if (!event_class) { - goto end; + goto error; } + stream_class = bt_ctf_event_class_get_stream_class(event_class); /* - * The event class does not keep ownership of the stream class to - * which it as been added. Therefore, it can't assume it has been - * set. However, we disallow the creation of an event if its - * associated stream class has been reclaimed. + * We disallow the creation of an event if its event class has not been + * associated to a stream class. */ - if (!event_class->stream_class) { - goto end; + if (!stream_class) { + goto error; } - assert(event_class->stream_class->event_header_type); + assert(stream_class->event_header_type); event = g_new0(struct bt_ctf_event, 1); if (!event) { - goto end; + goto error; } bt_object_init(event, bt_ctf_event_destroy); - bt_get(event_class); bt_ctf_event_class_freeze(event_class); - event->event_class = event_class; + /* + * event does not share a common ancestor with the event class; it has + * to guarantee its existence by holding a reference. This reference + * shall be released once the event is associated to a stream since, + * from that point, the event and its class will share the same + * lifetime. + */ + event->event_class = bt_get(event_class); event->event_header = bt_ctf_field_create( - event_class->stream_class->event_header_type); + stream_class->event_header_type); if (!event->event_header) { goto error; } @@ -583,11 +584,12 @@ struct bt_ctf_event *bt_ctf_event_create(struct bt_ctf_event_class *event_class) * Freeze the stream class since the event header must not be changed * anymore. */ - bt_ctf_stream_class_freeze(event_class->stream_class); -end: + bt_ctf_stream_class_freeze(stream_class); + BT_PUT(stream_class); return event; error: BT_PUT(event); + BT_PUT(stream_class); return event; } @@ -607,16 +609,7 @@ end: struct bt_ctf_stream *bt_ctf_event_get_stream(struct bt_ctf_event *event) { - struct bt_ctf_stream *stream = NULL; - - if (!event) { - goto end; - } - - stream = event->stream; - bt_get(stream); -end: - return stream; + return (struct bt_ctf_stream *) bt_object_get_parent(event); } struct bt_ctf_clock *bt_ctf_event_get_clock(struct bt_ctf_event *event) @@ -784,24 +777,21 @@ int bt_ctf_event_set_header(struct bt_ctf_event *event, { int ret = 0; struct bt_ctf_field_type *field_type = NULL; + struct bt_ctf_stream_class *stream_class = NULL; if (!event || !header) { ret = -1; goto end; } - /* Could be NULL since an event class doesn't own a stream class */ - if (!event->event_class->stream_class) { - ret = -1; - goto end; - } - + stream_class = (struct bt_ctf_stream_class *) bt_object_get_parent( + event->event_class); /* * Ensure the provided header's type matches the one registered to the * stream class. */ field_type = bt_ctf_field_get_type(header); - if (field_type != event->event_class->stream_class->event_header_type) { + if (field_type != stream_class->event_header_type) { ret = -1; goto end; } @@ -810,6 +800,7 @@ int bt_ctf_event_set_header(struct bt_ctf_event *event, bt_put(event->event_header); event->event_header = header; end: + bt_put(stream_class); bt_put(field_type); return ret; } @@ -886,7 +877,14 @@ void bt_ctf_event_destroy(struct bt_object *obj) struct bt_ctf_event *event; event = container_of(obj, struct bt_ctf_event, base); - bt_put(event->event_class); + if (!event->base.parent) { + /* + * Event was keeping a reference to its class since it shared no + * common ancestor with it to guarantee they would both have the + * same lifetime. + */ + bt_put(event->event_class); + } bt_put(event->event_header); bt_put(event->context_payload); bt_put(event->fields_payload); @@ -946,36 +944,6 @@ void bt_ctf_event_class_freeze(struct bt_ctf_event_class *event_class) bt_ctf_attributes_freeze(event_class->attributes); } -BT_HIDDEN -int bt_ctf_event_class_set_stream_class(struct bt_ctf_event_class *event_class, - struct bt_ctf_stream_class *stream_class) -{ - int ret = 0; - - if (!event_class) { - ret = -1; - goto end; - } - - /* Allow a NULL stream_class to unset the current stream_class */ - if (stream_class && event_class->stream_class) { - ret = -1; - goto end; - } - - event_class->stream_class = stream_class; - /* - * We don't get() the stream_class since doing so would introduce - * a circular ownership between event classes and stream classes. - * - * A stream class will always unset itself from its events before - * being destroyed. This ensures that a user won't get a pointer - * to a stale stream class instance from an event class. - */ -end: - return ret; -} - BT_HIDDEN int bt_ctf_event_class_serialize(struct bt_ctf_event_class *event_class, struct metadata_context *context) @@ -1194,28 +1162,6 @@ end: return ret; } -BT_HIDDEN -int bt_ctf_event_set_stream(struct bt_ctf_event *event, - struct bt_ctf_stream *stream) -{ - int ret = 0; - - if (!event) { - ret = -1; - goto end; - } - - if (event->stream && stream) { - /* Already attached to a stream */ - ret = -1; - goto end; - } - - event->stream = stream; -end: - return ret; -} - struct bt_ctf_event *bt_ctf_event_copy(struct bt_ctf_event *event) { struct bt_ctf_event *copy = NULL; @@ -1230,9 +1176,7 @@ struct bt_ctf_event *bt_ctf_event_copy(struct bt_ctf_event *event) } bt_object_init(copy, bt_ctf_event_destroy); - copy->event_class = event->event_class; - bt_get(copy->event_class); - copy->stream = event->stream; + copy->event_class = bt_get(event->event_class); if (event->event_header) { copy->event_header = bt_ctf_field_copy(event->event_header); diff --git a/formats/ctf/ir/stream-class.c b/formats/ctf/ir/stream-class.c index aa196a47..022675e3 100644 --- a/formats/ctf/ir/stream-class.c +++ b/formats/ctf/ir/stream-class.c @@ -65,7 +65,7 @@ struct bt_ctf_stream_class *bt_ctf_stream_class_create(const char *name) stream_class->name = g_string_new(name); stream_class->event_classes = g_ptr_array_new_with_free_func( - (GDestroyNotify) bt_put); + (GDestroyNotify) bt_object_release); if (!stream_class->event_classes) { goto error; } @@ -91,18 +91,8 @@ error: struct bt_ctf_trace *bt_ctf_stream_class_get_trace( struct bt_ctf_stream_class *stream_class) { - struct bt_ctf_trace *trace = NULL; - - if (!stream_class) { - goto end; - } - - trace = stream_class->trace; - if (trace) { - bt_get(trace); - } -end: - return trace; + return (struct bt_ctf_trace *) bt_object_get_parent( + stream_class); } const char *bt_ctf_stream_class_get_name( @@ -326,6 +316,8 @@ int bt_ctf_stream_class_add_event_class( { int ret = 0; int64_t event_id; + struct bt_ctf_trace *trace = NULL; + struct bt_ctf_stream_class *old_stream_class = NULL; if (!stream_class || !event_class) { ret = -1; @@ -341,15 +333,25 @@ int bt_ctf_stream_class_add_event_class( goto end; } + old_stream_class = (struct bt_ctf_stream_class *) bt_object_get_parent( + event_class); + if (old_stream_class) { + /* Event class is already associated to a stream class. */ + ret = -1; + goto end; + } + /* * Resolve the event's sequence length and variant tags if the * stream is already associated with a trace. Otherwise, this * validation will be performed once the stream is registered * to a trace. */ - if (stream_class->trace) { + trace = (struct bt_ctf_trace *) bt_object_get_parent( + stream_class); + if (trace) { ret = bt_ctf_event_class_resolve_types(event_class, - stream_class->trace, stream_class); + trace, stream_class); if (ret) { goto end; } @@ -365,17 +367,12 @@ int bt_ctf_stream_class_add_event_class( } } - ret = bt_ctf_event_class_set_stream_class(event_class, stream_class); - if (ret) { - goto end; - } - ret = bt_ctf_event_class_set_stream_id(event_class, stream_class->id); if (ret) { goto end; } - bt_get(event_class); + bt_object_set_parent(event_class, stream_class); g_ptr_array_add(stream_class->event_classes, event_class); bt_ctf_event_class_freeze(event_class); @@ -391,6 +388,8 @@ int bt_ctf_stream_class_add_event_class( stream_class->byte_order); } end: + BT_PUT(trace); + BT_PUT(old_stream_class); return ret; } @@ -733,28 +732,6 @@ end: return ret; } -BT_HIDDEN -int bt_ctf_stream_class_set_trace(struct bt_ctf_stream_class *stream_class, - struct bt_ctf_trace *trace) -{ - int ret = 0; - - if (!stream_class) { - ret = -1; - goto end; - } - - if (stream_class->trace && trace) { - /* Already attached to a trace */ - ret = -1; - goto end; - } - - stream_class->trace = trace; -end: - return ret; -} - static void bt_ctf_stream_class_destroy(struct bt_object *obj) { @@ -764,17 +741,6 @@ void bt_ctf_stream_class_destroy(struct bt_object *obj) bt_put(stream_class->clock); if (stream_class->event_classes) { - size_t i; - - /* Unregister this stream class from the event classes */ - for (i = 0; i < stream_class->event_classes->len; i++) { - struct bt_ctf_event_class *event_class = - g_ptr_array_index(stream_class->event_classes, - i); - - bt_ctf_event_class_set_stream_class(event_class, NULL); - } - g_ptr_array_free(stream_class->event_classes, TRUE); } diff --git a/formats/ctf/ir/stream.c b/formats/ctf/ir/stream.c index 84917140..6f972590 100644 --- a/formats/ctf/ir/stream.c +++ b/formats/ctf/ir/stream.c @@ -100,6 +100,7 @@ static int set_packet_header_uuid(struct bt_ctf_stream *stream) { int i, ret = 0; + struct bt_ctf_trace *trace = NULL; struct bt_ctf_field_type *uuid_field_type = NULL; struct bt_ctf_field_type *element_field_type = NULL; struct bt_ctf_field *uuid_field = bt_ctf_field_structure_get_field( @@ -140,6 +141,7 @@ int set_packet_header_uuid(struct bt_ctf_stream *stream) goto end; } + trace = (struct bt_ctf_trace *) bt_object_get_parent(stream); for (i = 0; i < 16; i++) { struct bt_ctf_field *uuid_element = bt_ctf_field_array_get_field(uuid_field, i); @@ -149,11 +151,11 @@ int set_packet_header_uuid(struct bt_ctf_stream *stream) if (ret) { ret = bt_ctf_field_signed_integer_set_value( - uuid_element, (int64_t) stream->trace->uuid[i]); + uuid_element, (int64_t) trace->uuid[i]); } else { ret = bt_ctf_field_unsigned_integer_set_value( uuid_element, - (uint64_t) stream->trace->uuid[i]); + (uint64_t) trace->uuid[i]); } bt_put(uuid_element); if (ret) { @@ -165,6 +167,7 @@ end: bt_put(uuid_field); bt_put(uuid_field_type); bt_put(element_field_type); + BT_PUT(trace); return ret; } static @@ -234,10 +237,19 @@ end: } static -void put_event(struct bt_ctf_event *event) +void release_event(struct bt_ctf_event *event) { - bt_ctf_event_set_stream(event, NULL); - bt_put(event); + if (bt_object_get_ref_count(event)) { + /* + * The event is being orphaned, but it must guarantee the + * existence of its event class for the duration of its + * lifetime. + */ + bt_get(event->event_class); + BT_PUT(event->base.parent); + } else { + bt_object_release(event); + } } BT_HIDDEN @@ -257,9 +269,12 @@ struct bt_ctf_stream *bt_ctf_stream_create( goto end; } - /* A stream has no ownership of its trace (weak ptr) */ - stream->trace = trace; bt_object_init(stream, bt_ctf_stream_destroy); + /* + * Acquire reference to parent since stream will become publicly + * reachable; it needs its parent to remain valid. + */ + bt_object_set_parent(stream, trace); stream->packet_context = bt_ctf_field_create( stream_class->packet_context_type); if (!stream->packet_context) { @@ -290,9 +305,8 @@ struct bt_ctf_stream *bt_ctf_stream_create( stream->pos.fd = -1; stream->id = stream_class->next_stream_id++; stream->stream_class = stream_class; - bt_get(stream_class); stream->events = g_ptr_array_new_with_free_func( - (GDestroyNotify) put_event); + (GDestroyNotify) release_event); if (!stream->events) { goto error; } @@ -326,6 +340,7 @@ end: return stream; error: BT_PUT(stream); + bt_put(trace); return stream; } @@ -490,13 +505,7 @@ int bt_ctf_stream_append_event(struct bt_ctf_stream *stream, goto end; } - ret = bt_ctf_event_set_stream(event, stream); - if (ret) { - /* Event was already associated to a stream */ - ret = -1; - goto end; - } - + bt_object_set_parent(event, stream); ret = bt_ctf_event_populate_event_header(event); if (ret) { goto end; @@ -523,15 +532,25 @@ int bt_ctf_stream_append_event(struct bt_ctf_stream *stream, } } - bt_get(event); /* Save the new event along with its associated stream event context */ g_ptr_array_add(stream->events, event); if (event_context_copy) { g_ptr_array_add(stream->event_contexts, event_context_copy); } + /* + * Event had to hold a reference to its event class as long as it wasn't + * part of the same trace hierarchy. From now on, the event and its + * class share the same lifetime guarantees and the reference is no + * longer needed. + */ + bt_put(event->event_class); end: if (ret) { - (void) bt_ctf_event_set_stream(event, NULL); + /* + * Orphan the event; we were not succesful in associating it to + * a stream. + */ + bt_object_set_parent(event, NULL); } return ret; } @@ -641,6 +660,7 @@ int bt_ctf_stream_set_packet_header(struct bt_ctf_stream *stream, struct bt_ctf_field *field) { int ret = 0; + struct bt_ctf_trace *trace = NULL; struct bt_ctf_field_type *field_type = NULL; if (!stream || !field) { @@ -648,8 +668,9 @@ int bt_ctf_stream_set_packet_header(struct bt_ctf_stream *stream, goto end; } + trace = (struct bt_ctf_trace *) bt_object_get_parent(stream); field_type = bt_ctf_field_get_type(field); - if (field_type != stream->trace->packet_header_type) { + if (field_type != trace->packet_header_type) { ret = -1; goto end; } @@ -658,6 +679,7 @@ int bt_ctf_stream_set_packet_header(struct bt_ctf_stream *stream, bt_put(stream->packet_header); stream->packet_header = field; end: + BT_PUT(trace); bt_put(field_type); return ret; } @@ -891,7 +913,6 @@ void bt_ctf_stream_destroy(struct bt_object *obj) perror("close"); } - bt_put(stream->stream_class); if (stream->events) { g_ptr_array_free(stream->events, TRUE); } diff --git a/formats/ctf/ir/trace.c b/formats/ctf/ir/trace.c index c2f8be48..70ac2b45 100644 --- a/formats/ctf/ir/trace.c +++ b/formats/ctf/ir/trace.c @@ -68,13 +68,6 @@ const unsigned int field_type_aliases_sizes[] = { [FIELD_TYPE_ALIAS_UINT64_T] = 64, }; -static -void put_stream_class(struct bt_ctf_stream_class *stream_class) -{ - (void) bt_ctf_stream_class_set_trace(stream_class, NULL); - bt_put(stream_class); -} - struct bt_ctf_trace *bt_ctf_trace_create(void) { struct bt_ctf_trace *trace = NULL; @@ -89,9 +82,9 @@ struct bt_ctf_trace *bt_ctf_trace_create(void) trace->clocks = g_ptr_array_new_with_free_func( (GDestroyNotify) bt_put); trace->streams = g_ptr_array_new_with_free_func( - (GDestroyNotify) bt_put); + (GDestroyNotify) bt_object_release); trace->stream_classes = g_ptr_array_new_with_free_func( - (GDestroyNotify) put_stream_class); + (GDestroyNotify) bt_object_release); if (!trace->clocks || !trace->stream_classes || !trace->streams) { goto error; } @@ -170,9 +163,7 @@ struct bt_ctf_stream *bt_ctf_trace_create_stream(struct bt_ctf_trace *trace, goto error; } - bt_get(stream); g_ptr_array_add(trace->streams, stream); - return stream; error: BT_PUT(stream); @@ -471,14 +462,7 @@ int bt_ctf_trace_add_stream_class(struct bt_ctf_trace *trace, } } - /* Set weak reference to trace in stream class */ - ret = bt_ctf_stream_class_set_trace(stream_class, trace); - if (ret) { - /* Stream class already part of another trace */ - goto end; - } - - bt_get(stream_class); + bt_object_set_parent(stream_class, trace); g_ptr_array_add(trace->stream_classes, stream_class); /* @@ -499,11 +483,10 @@ int bt_ctf_trace_add_stream_class(struct bt_ctf_trace *trace, bt_ctf_stream_class_freeze(stream_class); if (!trace->frozen) { ret = bt_ctf_trace_freeze(trace); - goto end; } end: if (ret) { - (void) bt_ctf_stream_class_set_trace(stream_class, NULL); + bt_object_set_parent(stream_class, NULL); } return ret; } diff --git a/formats/ctf/writer/writer.c b/formats/ctf/writer/writer.c index f8a36778..52c153d1 100644 --- a/formats/ctf/writer/writer.c +++ b/formats/ctf/writer/writer.c @@ -74,6 +74,8 @@ struct bt_ctf_writer *bt_ctf_writer_create(const char *path) goto error_destroy; } + bt_object_set_parent(writer->trace, writer); + bt_put(writer->trace); /* Create trace directory if necessary and open a metadata file */ if (g_mkdir_with_parents(path, S_IRWXU | S_IRWXG)) { perror("g_mkdir_with_parents"); @@ -121,7 +123,7 @@ void bt_ctf_writer_destroy(struct bt_object *obj) } } - bt_put(writer->trace); + bt_object_release(writer->trace); g_free(writer); } diff --git a/include/babeltrace/ctf-ir/event-internal.h b/include/babeltrace/ctf-ir/event-internal.h index 17695f92..857ea01d 100644 --- a/include/babeltrace/ctf-ir/event-internal.h +++ b/include/babeltrace/ctf-ir/event-internal.h @@ -43,11 +43,6 @@ struct bt_ctf_event_class { struct bt_object base; struct bt_value *attributes; - /* - * Weak reference; an event class does not have ownership of a - * stream class. - */ - struct bt_ctf_stream_class *stream_class; /* Structure type containing the event's context */ struct bt_ctf_field_type *context; /* Structure type containing the event's fields */ @@ -58,8 +53,6 @@ struct bt_ctf_event_class { struct bt_ctf_event { struct bt_object base; struct bt_ctf_event_class *event_class; - /* Weak reference; an event does not have ownership of a stream */ - struct bt_ctf_stream *stream; struct bt_ctf_field *event_header; struct bt_ctf_field *context_payload; struct bt_ctf_field *fields_payload; @@ -68,10 +61,6 @@ struct bt_ctf_event { BT_HIDDEN void bt_ctf_event_class_freeze(struct bt_ctf_event_class *event_class); -BT_HIDDEN -int bt_ctf_event_class_set_stream_class(struct bt_ctf_event_class *event_class, - struct bt_ctf_stream_class *stream_class); - BT_HIDDEN int bt_ctf_event_class_serialize(struct bt_ctf_event_class *event_class, struct metadata_context *context); @@ -92,10 +81,6 @@ BT_HIDDEN int bt_ctf_event_serialize(struct bt_ctf_event *event, struct ctf_stream_pos *pos); -BT_HIDDEN -int bt_ctf_event_set_stream(struct bt_ctf_event *event, - struct bt_ctf_stream *stream); - /* * Attempt to populate the "id" and "timestamp" fields of the event header if * they are present, unset and their types are integers. diff --git a/include/babeltrace/ctf-ir/stream-class-internal.h b/include/babeltrace/ctf-ir/stream-class-internal.h index 2a481b08..88d38c21 100644 --- a/include/babeltrace/ctf-ir/stream-class-internal.h +++ b/include/babeltrace/ctf-ir/stream-class-internal.h @@ -45,8 +45,6 @@ struct bt_ctf_stream_class { uint32_t id; uint32_t next_event_id; uint32_t next_stream_id; - /* Weak reference; a stream class does not have ownership of a trace */ - struct bt_ctf_trace *trace; struct bt_ctf_field_type *packet_context_type; struct bt_ctf_field_type *event_header_type; struct bt_ctf_field_type *event_context_type; @@ -74,8 +72,4 @@ BT_HIDDEN int bt_ctf_stream_class_set_id_no_check( struct bt_ctf_stream_class *stream_class, uint32_t id); -BT_HIDDEN -int bt_ctf_stream_class_set_trace(struct bt_ctf_stream_class *stream_class, - struct bt_ctf_trace *trace); - #endif /* BABELTRACE_CTF_IR_STREAM_CLASS_INTERNAL_H */ diff --git a/include/babeltrace/ctf-ir/stream-internal.h b/include/babeltrace/ctf-ir/stream-internal.h index ca22eb11..f2ba5d36 100644 --- a/include/babeltrace/ctf-ir/stream-internal.h +++ b/include/babeltrace/ctf-ir/stream-internal.h @@ -38,8 +38,6 @@ struct bt_ctf_stream { struct bt_object base; - /* Trace owning this stream. A stream does not own a trace. */ - struct bt_ctf_trace *trace; uint32_t id; struct bt_ctf_stream_class *stream_class; /* Array of pointers to bt_ctf_event for the current packet */ diff --git a/tests/lib/test_ctf_writer.c b/tests/lib/test_ctf_writer.c index 2d7c5a50..596bb90d 100644 --- a/tests/lib/test_ctf_writer.c +++ b/tests/lib/test_ctf_writer.c @@ -3285,9 +3285,6 @@ int main(int argc, char **argv) bt_put(packet_header_field); bt_put(trace); free(metadata_string); - - ok(bt_ctf_stream_class_get_trace(stream_class) == NULL, - "bt_ctf_stream_class_get_trace returns NULL after its trace has been reclaimed"); bt_put(stream_class); /* Remove all trace files and delete temporary trace directory */ -- 2.34.1