GNOME Bugzilla – Bug 732948
Add backend-per-process support
Last modified: 2014-09-30 09:22:23 UTC
See the attached path
Created attachment 280268 [details] [review] Bug #732948 - Add backend-per-process support With this we can have all backends running on its own processes, in this way, if a backend crashes we don't have the whole factory crashing together. A configure option was added and, in case the user wants to keep the old behavior, it can be disabled by: --disable-backend-per-process
Review of attachment 280268 [details] [review]: Apart of the below, Fidencio has ready a change which will not group backends by the factory name, but will have an option for it. ::: addressbook/libebook/e-book-client-view.c @@ +886,3 @@ + book_client = g_weak_ref_get (&priv->client); + bus_name = e_client_dup_bus_name (E_CLIENT (book_client)); The GWeakRef is there for a case that the client itself dies before the view. In that case the returned book_client will be NULL. Check for it and react accordingly, otherwise runtime warnings will be spread on the console instead. ::: addressbook/libedata-book/Makefile.am @@ +113,3 @@ + -I$(top_builddir)/addressbook \ + $(E_DATA_SERVER_CFLAGS) \ + $(EVOLUTION_ADDRESSBOOK_CFLAGS) \ I think you should use the same CFLAGS and LIBS as the main factories use, to keep the "eco-system" unchanged for the backends, but I'm not 100% sure. If you can make it this lightweight, then it'll be nice. (Fedora usually do not claim linking issues that consistently as other distros, like Debian or Ubuntu, if I recall correctly.) ::: addressbook/libedata-book/e-subprocess-book-factory.c @@ +1,3 @@ +/* -*- Mode: C; tab-width: 8; indent-tabs-mode: t; c-basic-offset: 8 -*- */ +/* + * Copyright (C) 2014 Red Hat I use: Copyright (C) 2014 Red Hat, Inc. (www.redhat.com) @@ +27,3 @@ + * with client side #EBookClient objects. + **/ +#include <config.h> Please do not forget of #ifdef HAVE_CONFIG_H ... #endif and an empty line above it would be also nice. @@ +32,3 @@ +#include <string.h> +#include <unistd.h> +#include <glib/gi18n.h> certainly gi18n-lib.h, due to this being a library ::: addressbook/libedata-book/e-subprocess-book-factory.h @@ +3,3 @@ + * Copyright (C) 1999-2008 Novell, Inc. (www.novell.com) + * Copyright (C) 2006 OpenedHand Ltd + * Copyright (C) 2009 Intel Corporation really that many contributors? This is a new file, thus treat it as such @@ +19,3 @@ + +#if !defined (__LIBEDATA_BOOK_H_INSIDE__) && !defined (LIBEDATA_BOOK_COMPILATION) +#error "Only <libesubprocess-book/libesubprocess-book.h> should be included directly." No such file or directory ::: addressbook/libedata-book/evolution-addressbook-factory-subprocess.c @@ +19,3 @@ +#include <locale.h> +#include <stdlib.h> +#include <glib/gi18n.h> config.h, but not the gi18n-lib.h, because this is the application, not a library (if it's more times in the code, then I'll not repeat it, just check/fix newly added files, please). ::: calendar/libecal/e-cal-client-view.c @@ +699,3 @@ + cal_client = g_weak_ref_get (&priv->client); + bus_name = e_client_dup_bus_name (E_CLIENT (cal_client)); recall the GWeakRef note from the book client view above ::: calendar/libedata-cal/Makefile.am @@ +86,3 @@ + -I$(top_builddir)/calendar \ + $(E_DATA_SERVER_CFLAGS) \ + $(EVOLUTION_CALENDAR_CFLAGS) \ recall eco-system (CFLAGS/LIBS) note from the book factory above ::: calendar/libedata-cal/e-subprocess-cal-factory.h @@ +19,3 @@ + +#if !defined (__LIBEDATA_CAL_H_INSIDE__) && !defined (LIBEDATA_CAL_COMPILATION) +#error "Only <libesubprocess-cal/libesubprocess-cal.h> should be included directly." No such file or directory. Other complains from the similar newly added files in addressbook apply here as well. ::: libebackend/e-data-factory.c @@ +146,3 @@ + g_return_val_if_fail (class->subprocess_bus_name_prefix != NULL, NULL); + + return g_strdup_printf ( please write here a comment/explanation whythe getpid() and the 'x' 'x' in the name is used here. @@ +323,3 @@ gboolean removed = FALSE; + /* If prox yis NULL, we remove all proxies for name. */ typo @@ +341,3 @@ + * As we can have more than one proxy being used by the same name, + * in the same array, we must remove the connections one by one + * and then notify the subprocess to quit itself when its the last it's @@ +808,3 @@ + + g_hash_table_destroy (priv->subprocess_watched_ids); + g_mutex_clear (&priv->subprocess_watched_ids_lock); g_mutex_init() call is missing for this mutex @@ +1046,2 @@ /** + * e_data_factory_spwan_subprocess_backend: typo in the function name @@ +1053,3 @@ * + * Spawns a new subprocess for a backend type and returns the object path + * of the new subprocess to the client, in the way the client can talks s/talks/talk/ ::: libebackend/e-module.c @@ +320,3 @@ + * @dirname: filename of the module to load + * + * Load the module from the specified filename into memory. If double space before 'If' ::: libebackend/e-subprocess-factory.c @@ +22,3 @@ + **/ + +#include "e-subprocess-factory.h" These (local headers) should be as the last, not as the first, in the code. @@ +208,3 @@ + * e_subprocess_factory_ref_initable_backend: + * @subprocess_factory: an #ESubprocessFactory + * @source: an #ESource no such parameter here, and others are missing as well @@ +317,3 @@ +/** + * e_subprocess_factory_construct_path: + * @subprocess_factory: an #ESubprocessFactory no such parameter here @@ +344,3 @@ + * @subprocess_factory: an #ESubprocessFactory + * @connection: a #GDBusConnection + * @sender: a string no sender @@ +425,3 @@ + } + + g_list_free (backends); as long as it's GList, and the return type matches, you can just g_list_foreach (backends, (...) g_object_ref, NULL); and return the 'backends' without reallocating the GList. @@ +482,3 @@ + * Install a toggle reference on the backend + * so we can signal it to shutdown once all + * client connections are close ...closed ::: libedataserver/e-client.c @@ +2139,3 @@ + const gchar *bus_name) +{ + client->priv->bus_name = g_strdup (bus_name); possible memory leak.
Created attachment 280304 [details] [review] Bug #732948 - Add backend-per-process support With this we can have all backends running on itw own processes, in this way, if a backend crashes we don't have the whole factory crahsing togheter. Also, each instance of the EBackendFactory can decide if they want to have all instances of the same backend (like calendar, memo lis t and task list) running in only one process (useful for evolution-ews, where we can share the connections between calendar, memo list and task list) or have each extension running on its own process. Apart from that, a configure option was added and, in case the user wants to keep the old behavior, it can be disabled by passing "--disable-backend-per-process" to the configure
Created attachment 280305 [details] [review] Adapt evolution-mapi to backend-per-process support
Created attachment 280306 [details] [review] Adapt evolution-mapi to support ackend-per-process
Created attachment 280307 [details] [review] Adapt evolution-mapi to support backend-per-process
Created attachment 280308 [details] [review] Adapt evolution-ews to support backend-per-process
Created attachment 280315 [details] [review] Adapt evolution-ews to support backend-per-process
Created attachment 280318 [details] [review] Adapt evolution-mapi to support backend-per-process
Review of attachment 280304 [details] [review]: Being picky, I was told that they spell "DBus" as "D-Bus". I'm going to test this. ::: addressbook/libebook/e-book-client-view.c @@ +887,3 @@ + book_client = g_weak_ref_get (&priv->client); + if (book_client == NULL) + return FALSE; You should populate also the error here, thus it's known why it failed. ::: calendar/libecal/e-cal-client-view.c @@ +700,3 @@ + cal_client = g_weak_ref_get (&priv->client); + if (cal_client == NULL) + return FALSE; similar as with the EBookClientView, populate error. ::: libebackend/e-backend-factory.h @@ +85,3 @@ + struct _EModule *e_module; + gboolean share_backends; 'share_backends' sounds to me like the factory should return the same EBackend instance for various types, while 'share_subprocess' seems clearer to what it does. ::: libebackend/e-data-factory.c @@ +26,2 @@ #include <config.h> +#include <glib/gi18n.h> keep gi18n-lib.h here, because there is no main() in this file.
Review of attachment 280318 [details] [review]: Please share subprocesses for mapi too, at least for now. Having them separate will provide quicker responses, but also 2*sources connections to the server, which can lead to "too many connections" error from the server easily.
Review of attachment 280315 [details] [review]: Looks fine, just take care of the rename of the share_backends variable.
Hmm, how do you run backends under valgrind? I can run the factory, but running the backends, which are always in the subprocess (also with the --disable-backend-per-process, which is expected), into where the valgrind doesn't follow (it doesn't check subprocesses), then it's quite tricky to do so. I thought I will disable backend per process, then only one subprocess will be always run. Then I'll see how the subprocess run look like, thus I run the factory and then replace the PID with the actual factory PID in bus name and own path, and then I run my application, but as I said, it's quite tricky to do it right. We should provide some easier way for the debugging of any kind. Actual debugging through environment variables works, because these are inherited to the subprocess, thus I'm concerned more about the valgrind or similar test tools.
(In reply to comment #13) > Hmm, how do you run backends under valgrind? I can run the factory, but running > the backends, which are always in the subprocess (also with the > --disable-backend-per-process, which is expected), into where the valgrind > doesn't follow (it doesn't check subprocesses), then it's quite tricky to do > so. > > I thought I will disable backend per process, then only one subprocess will be > always run. Then I'll see how the subprocess run look like, thus I run the > factory and then replace the PID with the actual factory PID in bus name and > own path, and then I run my application, but as I said, it's quite tricky to do > it right. > > We should provide some easier way for the debugging of any kind. Actual > debugging through environment variables works, because these are inherited to > the subprocess, thus I'm concerned more about the valgrind or similar test > tools. I use the option: --trace-children=yes.
Ah, thanks, I wasn't aware of the option. Two last things: a) please add to the subprocess mains (both in book and calendar) these lines: https://git.gnome.org/browse/evolution-data-server/tree/services/evolution-addressbook-factory/evolution-addressbook-factory.c#n94 which have its counterparts at the top of the file too. b) I have enabled an EWS account - it was disabled and I enabled it, but I see it only in mails, not in calendar or book, for some reason. I also tried to close/kill everything (especially source registry), but it did not help. I see a difference when I run the source registry from a console. The version from plain master shows things like: AUTH (1342640443.11122.23@zyxPad): Initiated my-ews: Pairing 1395759441.23632.39@zyxPad with resource AAMkAGFhNWJkY2VjLTZkMGYtNDAxZi05MzJiLWM5Nzk5YjNlZWE1MQAuAAAAAACiAADM9ruVQ6Evwv91CZjQAQAcr3hHVWVAS5inRJae6omjAAAAg5CdAAA= my-ews: Pairing 1395759441.23632.40@zyxPad with resource AAMkAGFhNWJkY2VjLTZkMGYtNDAxZi05MzJiLWM5Nzk5YjNlZWE1MQAuAAAAAACiAADM9ruVQ6Evwv91CZjQAQAcr3hHVWVAS5inRJae6omjAABctUYGAAA= .... immediately after start, while the patched eds (your branch) doesn't do that.
(In reply to comment #15) > while the patched eds (your branch) doesn't do that. The change at source_registry_server_source_added() is causing this, I believe.
Created attachment 280417 [details] [review] Adapt evolution-mapi to support backend-per-process
Created attachment 280418 [details] [review] Bug #732948 - Add backend-per-process support With this we can have all backends running on itw own processes, in this way, if a backend crashes we don't have the whole factory crahsing togheter. Also, each instance of the EBackendFactory can decide if they want to have all instances of the same backend (like calendar, memo lis t and task list) running in only one process (useful for evolution-ews, where we can share the connections between calendar, memo list and task list) or have each extension running on its own process. Apart from that, a configure option was added and, in case the user wants to keep the old behavior, it can be disabled by passing "--disable-backend-per-process" to the configure. As a side effect of these changes, we are enforcing that the hash-key used to keep track of the backend-factories will be built internally and that *always* will follow the "backend-name:extension-name" structure, even for ECollectionBackends.
Created attachment 280420 [details] [review] Adapt evolution-ews to support backend-per-process
Created attachment 280421 [details] [review] Adapt evolution-mapi to support backend-per-process
Created attachment 280422 [details] [review] Adapt evolution-ews to support backend-per-process
Review of attachment 280418 [details] [review]: It works fine, according to my tests. There is onyl one thing you wanted to assure, if the subprocess execute fails, then set the state back to NONE, thus any other pending openings could be proceeded. I did not face any such issue with my tests. Also verify that the wait for the cond to execute a subprocess doesn't block the main thread, because I also noticed this error: > Failed to open remote book: Unable to connect to 'DAViCal': Message did not receive a reply (timeout by message bus); took 13.567494 s This error suggests that the main thread of the factory is blocked by something. I was not reliably able to reproduce it. Can it be that the remote book/cal source is taking to long to open? We can deal with this later.
Review of attachment 280421 [details] [review]: Looks fine, ready for the master.
Review of attachment 280422 [details] [review]: Looks fine. Ready for the master. By the way, thank you for the work on this.
Created attachment 280471 [details] [review] Bug #732948 - Add backend-per-process support With this we can have all backends running on theirs own processes, in this way, if a backend crashes we don't have the whole factory crashing together. Also, each instance of the EBackendFactory can decide if they want to have all instances of the same backend (like calendar, memo list, and task list) running in only one process (useful for evolution-ews and evolution-mapi where we can share the connections between calendar, memo list and task list) or have each extension running on its own process. Apart from that, a configure option was added and, in case the user wants to keep the old behavior, it can be disabled by passing "--disable-backend-per-process" to the configure. As a side effect of these changes, we are enforcing that the hash-key used to keep track of the backend-factories will be built internally and that *always* will follow the "backend-name:extension-name" structure, even for ECollectionBackends.
All patches are pushed! EDS patch pushed as 3a4f47e..f3f1e94 (3.13.4+) evolution-ews patch pushed as 1b14ccf..dcb6d20 (3.13.4+) evolution-mapi patch pushed as ef8d4cd..28d18ab (3.13.4+)
Commit f3f1e94 appears to break DRA. See bug 737279.
Created attachment 287337 [details] typical benchmark result This also causes a noticeable performance regression. See attached image which is typical of the benchmark run (full results also available but I won't link them from bugzilla because they won't be there for long).
Created attachment 287360 [details] performance comparison Looks like we take the performance hit even with --disable-backend-per-process. In this chart 'master' is commit 3a4f47ec, immediately before the patch in question. The other two are both commit f3f1e94f with the later DRA fix, configured with/without backend-per-process support.
This is a bit confusing that D-Bus performance is actually worse with the multiple process approach. As I understood from Milan's reply to the email below, now the addressbook factory processes host their own private GDBusServers instead of pushing payloads through a running D-Bus daemon, so performance of D-Bus calls should actually have greatly improved by the change. For reference, discussion on this took place on gtk-app-devel-list: https://mail.gnome.org/archives/gtk-app-devel-list/2014-July/msg00016.html
(In reply to comment #30) > As I understood from Milan's reply to the email below, now the addressbook > factory processes host their own private GDBusServers instead of pushing > payloads through a running D-Bus daemon, so performance of D-Bus calls should > actually have greatly improved by the change. Not completely, the EDBusSubprocess [1] is generated as a GDBusObjectProxy. This is running in a separate process, which claims ownership of some given (D-Bus) name. This is using the standard D-Bus session, similar to other evolution factories. The difference is that the EBookClient asks the book factory to pen certain backend, thus the factory either returns an object path to an already runnig subprocess or spawns a new one and returns the path to it. the book client opens a connection to that object path and then does (most of) the operations directly to the subprocess. The subprocess lets know the factory that he backend is closed through a signal, but the factory can "force" a close too. Fidencio might describe this better, as he wrote it, I only reviewed it. [1] https://git.gnome.org/browse/evolution-data-server/tree/private/org.gnome.evolution.dataserver.Subprocess.Backend.xml