Created attachment 77788 [details] [review] Patch In contact_avatar_retrieved, some blocking I/O function calls are used. This causes, for example, gnome-shell to completely freeze under heavy disk I/O trying to cache avatars to disk (fsync() system calls take up to 0.8s on my system). This problem is especially noticeable on SSDs. The patch I attached should fix this problem. It also changes the test on contacts to wait for writes to have completed before checking the contents of the files.
I did not look at the patch in details yet, but that's something I should have done from the beginning indeed. Note that it could freeze UI only the first time you connect an account, it is not something that happens every days. Right?
Well, to me it happens quite often, I guess it depends on when gnome shell downloads the contact's avatar. I don't really know why, but it happened almost every time the disk was busy. You could find when it happens ptraceing gnome-shell
I think it'd be better to notify both avatar-file and avatar-mime-type simultaneously, and only when both have been written.
Created attachment 85832 [details] [review] This patch makes avatar_file and avatar_mime_type property notifiy signals sent together once both files have been written If this looks fine, I'll squash both commits to make it one coherent patch.
Thanks for looking into this! I'm reviewing a flattened version here. I'd suggest attaching a new, flattened version with these changes - it'll be easier to review that way. + write_data *avatar_data = g_slice_new (write_data); + write_data *mime_data = g_slice_new (write_data); + AvatarFileWrittenData *avatar_file_written_data = g_slice_new + (AvatarFileWrittenData); These don't need to be separate structs. I'd use a single struct to represent the whole operation: typedef struct { TpContact *self; gchar *token; gchar *mime_type; GFile *mime_file; } WriteAvatarData; (and if you discover you need more fields, you can add them). You don't actually need to copy the avatar data into this struct, as far as I can see, because you only use it for the first g_file_replace_contents_async() call (which will, internally, copy it). It looks as though you leak the memory for these structs in error code paths. I would suggest not allocating them until you need them (that would be just after the comment "Save avatar in cache..." here, I think) and then you won't need to worry about cleanup on errors. + avatar_file = g_file_new_for_path (filename); You don't unref this, so it's leaked. Unref it after calling g_file_replace_contents_async(). + g_free (avatar_data->str); + g_slice_free (write_data, avatar_data); + g_free (mime_data->str); + g_slice_free (write_data, mime_data); + g_object_unref (avatar_file_written_data->mime_file); + g_slice_free (AvatarFileWrittenData, avatar_file_written_data); This should be in a new function avatar_file_written_data_free() (or write_avatar_data_free() if you use the naming I suggested) so you don't have to keep duplicating it. I haven't reviewed all the duplicate copies of freeing the struct and its contents, so there might be memory leaks there - but when you do this, it'll be obvious at a glance whether things are being freed correctly. + if (avatar_data->self == NULL) + { + g_free (avatar_data->str); + g_slice_free (write_data, avatar_data); + g_free (mime_data->str); + g_slice_free (write_data, mime_data); + g_object_unref (avatar_file_written_data->mime_file); + g_slice_free (AvatarFileWrittenData, avatar_file_written_data); + return; + } + /* Update the avatar token if a newer one is given */ + contact_set_avatar_token (avatar_data->self, avatar_data->str, FALSE); + + tp_clear_object (&avatar_data->self->priv->avatar_file); + avatar_data->self->priv->avatar_file = (GFile *) file; + g_free (avatar_data->str); + g_slice_free (write_data, avatar_data); + + /* Now that the avatar-file has been written, write the mime-file. */ + g_file_replace_contents_async (avatar_file_written_data->mime_file, + mime_data->str, strlen (mime_data->str), NULL, FALSE, G_FILE_CREATE_NONE, + NULL, &mime_file_written, mime_data); + + g_slice_free (AvatarFileWrittenData, avatar_file_written_data); There's a logic error here: if the TpContact is NULL, you don't write the MIME type to a file. If you're signalling the avatar token here, I'd do: if (data->self != NULL) { contact_set_avatar_token (data->self, data->token, FALSE); } g_file_replace_contents_async(...) but actually it'd make more sense to delay signalling the changed avatar token until the end as well - the application can't usefully act on it until you've cached the file. If writing the avatar data fails, it's probably still worth trying to write out the MIME type, and if either or both fail, I think it's still worth signalling the token change (and perhaps also the MIME type and avatar-file), even though the current implementation didn't. That simplifies things down to one code path. General points -------------- In both g_file_replace_contents_async calls, I think you should use G_FILE_CREATE_PRIVATE|G_FILE_CREATE_REPLACE_DESTINATION for the overwrite - we're potentially dealing with private data here, and REPLACE_DESTINATION gives the closest match for how g_file_set_contents() worked. Minor nitpicking stuff ---------------------- +typedef struct{ Coding style: space before "{". +static void mime_file_written (GObject *file, + GAsyncResult *res, + gpointer user_data){ Coding style: newlines after "static void" and ")" here. We also usually indent the second and subsequent arguments in function definitions by 4 spaces, rather than lining them up with the "(" - please follow <http://telepathy.freedesktop.org/wiki/Style/> and/or the style of nearby code. + GError* error = NULL; + write_data* mime_data = (write_data *) user_data; Coding style: "type *value" not "type* value" (<http://lists.freedesktop.org/archives/systemd-devel/2013-April/010753.html> for the reason). You don't need a cast when going from gpointer (or equivalently, void *) to SomeType *. There are a lot of (GFile *) casts here. I'd prefer this: static void something_written (GObject *source_object, GAsyncResult *res, gpointer user_data) { GFile *file = G_FILE (source_object); WriteFileData *data = user_data; GError *error = NULL; if (!g_file_replace_contents_finish (file, res, NULL, &error)) { ... } ... (and the same for all other uses of the GFile) ... } Coding style: blank line after the variable declarations, please (like in the example immediately before this comment). Coding style: it's conventional to check the return from the _finish function, rather than the contents of 'error', as in the same example. If it fails, it's OK to assume that error is non-NULL.
(In reply to comment #5) > Thanks for looking into this! > > I'm reviewing a flattened version here. I'd suggest attaching a new, > flattened version with these changes - it'll be easier to review that way. > > + write_data *avatar_data = g_slice_new (write_data); > + write_data *mime_data = g_slice_new (write_data); > + AvatarFileWrittenData *avatar_file_written_data = g_slice_new > + (AvatarFileWrittenData); > > These don't need to be separate structs. I'd use a single struct to > represent the whole operation: > > typedef struct { > TpContact *self; > gchar *token; > gchar *mime_type; > GFile *mime_file; > } WriteAvatarData; > > (and if you discover you need more fields, you can add them). Done. > > You don't actually need to copy the avatar data into this struct, as far as > I can see, because you only use it for the first > g_file_replace_contents_async() call (which will, internally, copy it). I am not quite acquainted with GTask but if I remove the avatar data the test ./tests/dbus/test-contacts which checks this code-path fails with error: ERROR:contacts.c:599:create_contact_with_fake_avatar: assertion failed (content == avatar_data): ("\210\177*\b-avatar-data" == "fake-avatar-data") > > It looks as though you leak the memory for these structs in error code > paths. I would suggest not allocating them until you need them (that would > be just after the comment "Save avatar in cache..." here, I think) and then > you won't need to worry about cleanup on errors. > > + avatar_file = g_file_new_for_path (filename); > > You don't unref this, so it's leaked. Unref it after calling > g_file_replace_contents_async(). Done, but how do you know g_file_replace_contents_async() keeps it's own reference? Also, the parameter is called "input file". > > + g_free (avatar_data->str); > + g_slice_free (write_data, avatar_data); > + g_free (mime_data->str); > + g_slice_free (write_data, mime_data); > + g_object_unref (avatar_file_written_data->mime_file); > + g_slice_free (AvatarFileWrittenData, avatar_file_written_data); > > This should be in a new function avatar_file_written_data_free() (or > write_avatar_data_free() if you use the naming I suggested) so you don't > have to keep duplicating it. > > I haven't reviewed all the duplicate copies of freeing the struct and its > contents, so there might be memory leaks there - but when you do this, it'll > be obvious at a glance whether things are being freed correctly. I guess this isn't required now. > > + if (avatar_data->self == NULL) > + { > + g_free (avatar_data->str); > + g_slice_free (write_data, avatar_data); > + g_free (mime_data->str); > + g_slice_free (write_data, mime_data); > + g_object_unref (avatar_file_written_data->mime_file); > + g_slice_free (AvatarFileWrittenData, avatar_file_written_data); > + return; > + } > + /* Update the avatar token if a newer one is given */ > + contact_set_avatar_token (avatar_data->self, avatar_data->str, FALSE); > + > + tp_clear_object (&avatar_data->self->priv->avatar_file); > + avatar_data->self->priv->avatar_file = (GFile *) file; > + g_free (avatar_data->str); > + g_slice_free (write_data, avatar_data); > + > + /* Now that the avatar-file has been written, write the mime-file. */ > + g_file_replace_contents_async (avatar_file_written_data->mime_file, > + mime_data->str, strlen (mime_data->str), NULL, FALSE, > G_FILE_CREATE_NONE, > + NULL, &mime_file_written, mime_data); > + > + g_slice_free (AvatarFileWrittenData, avatar_file_written_data); > > There's a logic error here: if the TpContact is NULL, you don't write the > MIME type to a file. If you're signalling the avatar token here, I'd do: > > if (data->self != NULL) > { > contact_set_avatar_token (data->self, data->token, FALSE); > } > > g_file_replace_contents_async(...) > > but actually it'd make more sense to delay signalling the changed avatar > token until the end as well - the application can't usefully act on it until > you've cached the file. > > If writing the avatar data fails, it's probably still worth trying to write > out the MIME type, and if either or both fail, I think it's still worth > signalling the token change (and perhaps also the MIME type and > avatar-file), even though the current implementation didn't. That simplifies > things down to one code path. > > General points > -------------- > > In both g_file_replace_contents_async calls, I think you should use > G_FILE_CREATE_PRIVATE|G_FILE_CREATE_REPLACE_DESTINATION for the overwrite - > we're potentially dealing with private data here, and REPLACE_DESTINATION > gives the closest match for how g_file_set_contents() worked. > > Minor nitpicking stuff > ---------------------- > > +typedef struct{ > > Coding style: space before "{". > > +static void mime_file_written (GObject *file, > + GAsyncResult *res, > + gpointer user_data){ > > Coding style: newlines after "static void" and ")" here. We also usually > indent the second and subsequent arguments in function definitions by 4 > spaces, rather than lining them up with the "(" - please follow > <http://telepathy.freedesktop.org/wiki/Style/> and/or the style of nearby > code. > > + GError* error = NULL; > + write_data* mime_data = (write_data *) user_data; > > Coding style: "type *value" not "type* value" > (<http://lists.freedesktop.org/archives/systemd-devel/2013-April/010753. > html> for the reason). > > You don't need a cast when going from gpointer (or equivalently, void *) to > SomeType *. > > There are a lot of (GFile *) casts here. I'd prefer this: > > static void > something_written (GObject *source_object, > GAsyncResult *res, > gpointer user_data) > { > GFile *file = G_FILE (source_object); > WriteFileData *data = user_data; > GError *error = NULL; > > if (!g_file_replace_contents_finish (file, res, NULL, &error)) > { > ... > } > > ... (and the same for all other uses of the GFile) ... > } > > Coding style: blank line after the variable declarations, please (like in > the example immediately before this comment). > > Coding style: it's conventional to check the return from the _finish > function, rather than the contents of 'error', as in the same example. If it > fails, it's OK to assume that error is non-NULL. Fixed. Patch follows. It seems that the test required some changes though, I haven't looked into it yet.
Created attachment 85964 [details] [review] Updated patch
Uh, wait! I get a : GLib-GIO-CRITICAL **: g_file_load_contents: assertion 'G_IS_FILE (file)' failed on avatar_loaded signal with this patch.
(In reply to comment #8) > Uh, wait! > I get a : > GLib-GIO-CRITICAL **: g_file_load_contents: assertion 'G_IS_FILE (file)' > failed > on avatar_loaded signal with this patch. AvatarRetrieved signal I mean.
Comment on attachment 85964 [details] [review] Updated patch Review of attachment 85964 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/contact.c @@ +2814,5 @@ > + GFile *file = G_FILE (source_object); > + gchar *mime_filename = NULL; > + GFile *mime_file = NULL; > + > + g_array_unref (avatar_data->data); I'd leave this in the struct, and do all the freeing in one go, at the end. If you do want to free it here, use tp_clear_pointer or g_clear_pointer (or explicitly set avatar_data->data to NULL) so you aren't pointing to already-freed memory. @@ +2830,5 @@ > + > + if (avatar_data->self != NULL) > + { > + tp_clear_object (&avatar_data->self->priv->avatar_file); > + avatar_data->self->priv->avatar_file = file; You need to ref this when you store it in priv->avatar_file. That probably explains the critical you saw. @@ +2836,5 @@ > + > + /* Now write the mime-file. */ > + if (!build_avatar_filename (avatar_data->connection, avatar_data->token, > + TRUE, NULL, &mime_filename)) > + return; In this early-return path, you don't free avatar_data or signal the change. (But why would this fail anyway?) I would suggest going back to doing a single build_avatar_filename() call in contact_avatar_retrieved() for both the avatar filename and the MIME-type filename, and putting either the MIME-type's path or its GFile in the struct (I'd use the GFile, actually). That way, there's no way you can get an unexpected failure here, so you don't need the ability to do an early-return. @@ +2861,5 @@ > { > TpContact *self = _tp_connection_lookup_contact (connection, handle); > + gchar *filename = NULL; > + GFile *avatar_file = NULL; > + WriteAvatarData *avatar_data = g_slice_new (WriteAvatarData); I generally use g_slice_new0 so that if I forget to initialize a new field, it's 0 or NULL, and I get a more deterministic crash (or even correct behaviour, if NULL initialization was correct). Declare this up here, but initialize it further down: otherwise... @@ +2868,1 @@ > return; ... early-returning here (which happens if g_mkdir_with_parents() fails) leaks the WriteAvatarData. @@ +2873,3 @@ > > + avatar_data->data = g_array_new (FALSE, FALSE, sizeof (gchar)); > + g_array_append_vals (avatar_data->data, avatar->data, avatar->len); The reason you have to copy this (which, you're quite right, you do) is that g_file_replace_contents_async() doesn't copy it. I think that's a GIO bug, which we should open - how is Python/JS code expected to ensure that the content "stays alive" like this? - but this is an appropriate way to work around it. @@ +2873,4 @@ > > + avatar_data->data = g_array_new (FALSE, FALSE, sizeof (gchar)); > + g_array_append_vals (avatar_data->data, avatar->data, avatar->len); > + avatar_data->self = self; This should either ref it, or take a weak reference, if non-null. Otherwise, things will go horribly wrong when you try to use it if it's already been freed. I'd suggest a GWeakRef, because the code being called already "does the right thing" if there is no TpContact. @@ +2873,5 @@ > > + avatar_data->data = g_array_new (FALSE, FALSE, sizeof (gchar)); > + g_array_append_vals (avatar_data->data, avatar->data, avatar->len); > + avatar_data->self = self; > + avatar_data->connection = connection; This should ref it (because the code being called can't cope with there being no connection). @@ +2880,5 @@ > > + g_file_replace_contents_async (avatar_file, avatar_data->data->data, > + avatar_data->data->len, NULL, FALSE, > + G_FILE_CREATE_PRIVATE|G_FILE_CREATE_REPLACE_DESTINATION, NULL, > + &avatar_file_written, avatar_data); Style: you don't need the "&" to point to a function, and GLib code normally doesn't. Just quoting the name of a function is enough to create a function pointer. ::: tests/dbus/contacts.c @@ +585,5 @@ > g_main_loop_run (result.loop); > } > > + while (g_main_context_pending (NULL)) > + g_main_context_iteration (NULL, TRUE); I'd prefer this to wait for whichever signal you emit last (which I think is notify::avatar-file here).
(In reply to comment #6) > > + avatar_file = g_file_new_for_path (filename); > > > > You don't unref this, so it's leaked. Unref it after calling > > g_file_replace_contents_async(). > > Done, but how do you know g_file_replace_contents_async() keeps it's own > reference? Because GAsyncResult functions (those that have a non-NULL source object, anyway) always do. g_async_result_get_source_object returns the source object, so any correct implementation of that API must be keeping a ref to that source object. (This convention is why I was surprised that you turned out to be right about the avatar data - I expected the function to copy that, too, so that the caller didn't have to keep it around.) > > This should be in a new function avatar_file_written_data_free() (or > > write_avatar_data_free() if you use the naming I suggested) so you don't > > have to keep duplicating it. > > I guess this isn't required now. If you only have one code path by getting rid of the early-return (after writing out the avatar but without writing out the MIME type): no, it isn't needed (although I often split it out anyway - the compiler will inline it if appropriate).
(In reply to comment #10) > > + avatar_data->data = g_array_new (FALSE, FALSE, sizeof (gchar)); > > + g_array_append_vals (avatar_data->data, avatar->data, avatar->len); > > The reason you have to copy this (which, you're quite right, you do) is that > g_file_replace_contents_async() doesn't copy it. > > I think that's a GIO bug, which we should open - how is Python/JS code > expected to ensure that the content "stays alive" like this? - but this is > an appropriate way to work around it. https://bugzilla.gnome.org/show_bug.cgi?id=690525
(In reply to comment #12) > https://bugzilla.gnome.org/show_bug.cgi?id=690525 Actually, please add a comment "g_file_replace_contents_async() doesn't copy its argument, see <https://bugzilla.gnome.org/show_bug.cgi?id=690525>" (or something) next to where you copy the avatar data - we should get rid of that workaround if/when the GIO bug is fixed.
> ::: tests/dbus/contacts.c > @@ +585,5 @@ > > g_main_loop_run (result.loop); > > } > > > > + while (g_main_context_pending (NULL)) > > + g_main_context_iteration (NULL, TRUE); > > I'd prefer this to wait for whichever signal you emit last (which I think is > notify::avatar-file here). How can I do that?
(In reply to comment #14) > > I'd prefer this to wait for whichever signal you emit last (which I think is > > notify::avatar-file here). > > How can I do that? Something like this (pseudocode): static void notify_avatar_file_cb (GObject *object, GParamSpec *spec, gboolean *notified) { g_assert (!*notified); *notified = TRUE; } gboolean notified = FALSE; gulong notify_id; notify_id = g_signal_connect (contact, "notify::avatar-file", G_CALLBACK (notify_avatar_file_cb), ¬ified); while (!notified) g_main_context_iteration (NULL, TRUE); disconnect (contact, notify_id);
Created attachment 86443 [details] [review] Make avatar image caching asynchronous Based on patches by Luca Versari and Chandni Verma. --- I took this over because GNOME Shell developers are pretty keen to have it in 0.22. As well as fixing my review comments on Chandni's Attachment #85964 [details], I did some slight refactoring to avoid altering any of the TpContact's fields until the whole operation has finished, so that they're always consistent with whether notify has been emitted.
Comment on attachment 86443 [details] [review] Make avatar image caching asynchronous Review of attachment 86443 [details] [review]: ----------------------------------------------------------------- ++
Fixed in 0.21.2. Thanks to everyone who contributed to this :-)
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.