From 5974d2d08b2d4dffffd348d240aba8afb9dfeb35 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Mon, 11 Dec 2017 17:11:14 +1100 Subject: Service: filter changesets when performing writes such that changed events are only emitted if new values differ from existing values --- common/dconf-changeset.c | 70 +++++++++++++++++++++++++++++++++++++----------- common/dconf-changeset.h | 3 +++ service/dconf-writer.c | 24 ++++++++++------- 3 files changed, 72 insertions(+), 25 deletions(-) diff --git a/common/dconf-changeset.c b/common/dconf-changeset.c index c80c88c..705fec9 100644 --- a/common/dconf-changeset.c +++ b/common/dconf-changeset.c @@ -771,6 +771,57 @@ dconf_changeset_change (DConfChangeset *changeset, } } +/** + * dconf_changeset_filter_changes: + * @base: a database mode changeset + * @changes: a changeset + * + * Produces a changeset that contains all the changes in @changes that + * are not already present in @base + * + * If there are no such changes, %NULL is returned + * + * Applying the result to @base will yield the same result as applying + * @changes to @base + * + * Returns: (transfer full) (nullable): the minimal changes, or %NULL + * + * Since: 0.35.1 + */ +DConfChangeset * +dconf_changeset_filter_changes (DConfChangeset *base, + DConfChangeset *changes) +{ + DConfChangeset *result = NULL; + GHashTableIter iter; + gpointer key, val; + + g_return_val_if_fail (base->is_database, NULL); + + /* We create the list of changes by iterating the 'changes' changeset + * and noting any keys that are not in the 'base' changeset or do not + * have the same value in the 'base' changeset + * + * Note: because 'base' is a database changeset we don't have to + * worry about it containing NULL values (dir resets). + */ + g_hash_table_iter_init (&iter, changes->table); + while (g_hash_table_iter_next (&iter, &key, &val)) + { + GVariant *base_val = g_hash_table_lookup (base->table, key); + + if (base_val == NULL || !g_variant_equal (val, base_val)) + { + if (!result) + result = dconf_changeset_new (); + + dconf_changeset_set (result, key, val); + } + } + + return result; +} + /** * dconf_changeset_diff: * @from: a database mode changeset @@ -793,7 +844,7 @@ DConfChangeset * dconf_changeset_diff (DConfChangeset *from, DConfChangeset *to) { - DConfChangeset *changeset = NULL; + DConfChangeset *changeset; GHashTableIter iter; gpointer key, val; @@ -806,8 +857,8 @@ dconf_changeset_diff (DConfChangeset *from, * * We create our list of changes in two steps: * - * - iterate the 'to' changeset and note any keys that do not have - * the same value in the 'from' changeset + * - call dconf_changeset_filter_changes to find values from 'to' + * which are not present in 'from' or hold different values to 'to' * * - iterate the 'from' changeset and note any keys not present in * the 'to' changeset, recording resets for them @@ -817,19 +868,8 @@ dconf_changeset_diff (DConfChangeset *from, * Note: because 'from' and 'to' are database changesets we don't have * to worry about seeing NULL values or dirs. */ - g_hash_table_iter_init (&iter, to->table); - while (g_hash_table_iter_next (&iter, &key, &val)) - { - GVariant *from_val = g_hash_table_lookup (from->table, key); - if (from_val == NULL || !g_variant_equal (val, from_val)) - { - if (!changeset) - changeset = dconf_changeset_new (); - - dconf_changeset_set (changeset, key, val); - } - } + changeset = dconf_changeset_filter_changes (from, to); g_hash_table_iter_init (&iter, from->table); while (g_hash_table_iter_next (&iter, &key, &val)) diff --git a/common/dconf-changeset.h b/common/dconf-changeset.h index b0ce450..6fe60f2 100644 --- a/common/dconf-changeset.h +++ b/common/dconf-changeset.h @@ -65,6 +65,9 @@ DConfChangeset * dconf_changeset_deserialise (GVarian void dconf_changeset_change (DConfChangeset *changeset, DConfChangeset *changes); +DConfChangeset * dconf_changeset_filter_changes (DConfChangeset *from, + DConfChangeset *changes); + DConfChangeset * dconf_changeset_diff (DConfChangeset *from, DConfChangeset *to); diff --git a/service/dconf-writer.c b/service/dconf-writer.c index 26f66dd..8b59019 100644 --- a/service/dconf-writer.c +++ b/service/dconf-writer.c @@ -130,21 +130,25 @@ dconf_writer_real_change (DConfWriter *writer, const gchar *tag) { g_return_if_fail (writer->priv->uncommited_values != NULL); + DConfChangeset *effective_changeset = dconf_changeset_filter_changes (writer->priv->uncommited_values, + changeset); - dconf_changeset_change (writer->priv->uncommited_values, changeset); - - if (tag) + if (effective_changeset) { - TaggedChange *change; + dconf_changeset_change (writer->priv->uncommited_values, effective_changeset); + if (tag) + { + TaggedChange *change; - change = g_slice_new (TaggedChange); - change->changeset = dconf_changeset_ref (changeset); - change->tag = g_strdup (tag); + change = g_slice_new (TaggedChange); + change->changeset = dconf_changeset_ref (effective_changeset); + change->tag = g_strdup (tag); - g_queue_push_tail (&writer->priv->uncommited_changes, change); - } + g_queue_push_tail (&writer->priv->uncommited_changes, change); + } - writer->priv->need_write = TRUE; + writer->priv->need_write = TRUE; + } } static gboolean -- cgit v1.2.1 From 2206d4955184f45e47c3407d4af7fd46474cdbdb Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Wed, 17 Jan 2018 22:38:41 +1100 Subject: Service: Add unit tests for dconf_changeset_filter_changes --- tests/changeset.c | 126 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/tests/changeset.c b/tests/changeset.c index 5f046df..d22dcb8 100644 --- a/tests/changeset.c +++ b/tests/changeset.c @@ -572,6 +572,131 @@ test_diff (void) } } +static DConfChangeset * +changeset_from_string (const gchar *string, gboolean is_database) +{ + GVariant *parsed; + DConfChangeset *changes, *parsed_changes; + + if (is_database) + changes = dconf_changeset_new_database (NULL); + else + changes = dconf_changeset_new (); + + if (string != NULL) + { + parsed = g_variant_parse (NULL, string, NULL, NULL, NULL); + parsed_changes = dconf_changeset_deserialise (parsed); + dconf_changeset_change (changes, parsed_changes); + dconf_changeset_unref (parsed_changes); + g_variant_unref (parsed); + } + + return changes; +} + +static gchar * +string_from_changeset (DConfChangeset *changeset) +{ + GVariant *serialised; + gchar *string; + + if (dconf_changeset_is_empty (changeset)) + return NULL; + + serialised = dconf_changeset_serialise (changeset); + string = g_variant_print (serialised, TRUE); + g_variant_unref (serialised); + return string; +} + +static gchar* +call_filter_changes (const gchar *base_string, const gchar *changes_string) +{ + DConfChangeset *base, *changes, *filtered; + gchar *filtered_string = NULL; + + base = changeset_from_string (base_string, TRUE); + changes = changeset_from_string (changes_string, FALSE); + filtered = dconf_changeset_filter_changes (base, changes); + if (filtered != NULL) + { + filtered_string = string_from_changeset (filtered); + dconf_changeset_unref (filtered); + } + + dconf_changeset_unref (base); + dconf_changeset_unref (changes); + return filtered_string; +} + +static void +test_filter_changes (void) +{ + /* These tests are mostly negative, since dconf_changeset_filter_changes + * is called from dconf_changeset_diff */ + + // Define test changesets as serialised g_variant strings + const gchar *empty = NULL; + const gchar *a1 = "{'/a': @mv <'value1'>}"; + const gchar *a_null = "{'/a': @mv nothing}"; + const gchar *a2 = "{'/a': @mv <'value2'>}"; + const gchar *b2 = "{'/b': @mv <'value2'>}"; + const gchar *a1b1 = "{'/a': @mv <'value1'>, '/b': @mv <'value1'>}"; + const gchar *a1b2 = "{'/a': @mv <'value1'>, '/b': @mv <'value2'>}"; + const gchar *reset = "{'/': @mv nothing}"; + gchar *filtered; + + /* an empty changeset would not change an empty database */ + g_assert_null (call_filter_changes (empty, empty)); + + /* an empty changeset would not change a database with values */ + g_assert_null (call_filter_changes (a1, empty)); + + /* a changeset would not change a database with the same values */ + g_assert_null (call_filter_changes (a1, a1)); + g_assert_null (call_filter_changes (a1b2, a1b2)); + + /* A non-empty changeset would change an empty database */ + filtered = call_filter_changes (empty, a1); + g_assert_cmpstr (filtered, ==, a1); + g_free (filtered); + + /* a changeset would change a database with the same keys but different values */ + filtered = call_filter_changes (a1, a2); + g_assert_cmpstr (filtered, ==, a2); + g_free (filtered); + filtered = call_filter_changes (a1b1, a1b2); + g_assert_cmpstr (filtered, ==, b2); + g_free (filtered); + + /* A changeset would change a database with disjoint values */ + filtered = call_filter_changes (a1, b2); + g_assert_cmpstr (filtered, ==, b2); + g_free (filtered); + + /* A changeset would change a database with some equal and some new values */ + filtered = call_filter_changes (a1, a1b2); + g_assert_cmpstr (filtered, ==, b2); + g_free (filtered); + + /* A changeset would not change a database with some equal and some new values */ + g_assert_null (call_filter_changes (a1b2, a1)); + + /* Resets should count when there are values to be reset */ + filtered = call_filter_changes (a_null, reset); + g_assert_cmpstr (filtered, ==, reset); + g_free (filtered); + filtered = call_filter_changes (a1b2, reset); + g_assert_cmpstr (filtered, ==, reset); + g_free (filtered); + + /* FIXME: ideally, a reset would have no effect on an empty database */ + filtered = call_filter_changes (empty, reset); + g_assert_cmpstr (filtered, ==, reset); + g_free (filtered); +} + int main (int argc, char **argv) { @@ -584,6 +709,7 @@ main (int argc, char **argv) g_test_add_func ("/changeset/serialiser", test_serialiser); g_test_add_func ("/changeset/change", test_change); g_test_add_func ("/changeset/diff", test_diff); + g_test_add_func ("/changeset/filter", test_filter_changes); return g_test_run (); } -- cgit v1.2.1 From 77f799922ab6afbaeb3f7ee41c96548439fd3dbd Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Mon, 13 Aug 2018 14:25:47 +1000 Subject: Changeset: make dconf_changeset_filter_changes filter out key/path resets when appropriate --- common/dconf-changeset.c | 38 +++++++++++++++++++++++--- tests/changeset.c | 69 +++++++++++++++++++++++++++++++++++++++--------- 2 files changed, 90 insertions(+), 17 deletions(-) diff --git a/common/dconf-changeset.c b/common/dconf-changeset.c index 705fec9..cea34c1 100644 --- a/common/dconf-changeset.c +++ b/common/dconf-changeset.c @@ -793,7 +793,7 @@ dconf_changeset_filter_changes (DConfChangeset *base, DConfChangeset *changes) { DConfChangeset *result = NULL; - GHashTableIter iter; + GHashTableIter iter_changes; gpointer key, val; g_return_val_if_fail (base->is_database, NULL); @@ -805,13 +805,43 @@ dconf_changeset_filter_changes (DConfChangeset *base, * Note: because 'base' is a database changeset we don't have to * worry about it containing NULL values (dir resets). */ - g_hash_table_iter_init (&iter, changes->table); - while (g_hash_table_iter_next (&iter, &key, &val)) + g_hash_table_iter_init (&iter_changes, changes->table); + while (g_hash_table_iter_next (&iter_changes, &key, &val)) { GVariant *base_val = g_hash_table_lookup (base->table, key); - if (base_val == NULL || !g_variant_equal (val, base_val)) + if (g_str_has_suffix (key, "/")) + { + // Path reset + gboolean reset_is_effective = FALSE; + GHashTableIter iter_base; + gpointer base_key = NULL; + + g_return_val_if_fail (val == NULL, NULL); + + // First we check whether there are any keys in base that would be reset + g_hash_table_iter_init (&iter_base, base->table); + while (g_hash_table_iter_next (&iter_base, &base_key, NULL)) + if (g_str_has_prefix (base_key, key) && !g_str_equal (base_key, key)) + { + reset_is_effective = TRUE; + break; + } + + if (reset_is_effective) + { + if (!result) + result = dconf_changeset_new (); + + dconf_changeset_set (result, key, val); + } + } + else if (base_val == NULL && val == NULL) + continue; // Resetting a key that wasn't set + else if (val == NULL || base_val == NULL || !g_variant_equal (val, base_val)) { + // Resetting an existing key, inserting a value under a key that was not + // set, or replacing an existing value with a different one. if (!result) result = dconf_changeset_new (); diff --git a/tests/changeset.c b/tests/changeset.c index d22dcb8..d75e8d0 100644 --- a/tests/changeset.c +++ b/tests/changeset.c @@ -639,12 +639,14 @@ test_filter_changes (void) // Define test changesets as serialised g_variant strings const gchar *empty = NULL; const gchar *a1 = "{'/a': @mv <'value1'>}"; - const gchar *a_null = "{'/a': @mv nothing}"; const gchar *a2 = "{'/a': @mv <'value2'>}"; const gchar *b2 = "{'/b': @mv <'value2'>}"; const gchar *a1b1 = "{'/a': @mv <'value1'>, '/b': @mv <'value1'>}"; const gchar *a1b2 = "{'/a': @mv <'value1'>, '/b': @mv <'value2'>}"; - const gchar *reset = "{'/': @mv nothing}"; + const gchar *a1r1 = "{'/a': @mv <'value1'>, '/r/c': @mv <'value3'>}"; + const gchar *key_reset = "{'/a': @mv nothing}"; + const gchar *root_reset = "{'/': @mv nothing}"; + const gchar *partial_reset = "{'/r/': @mv nothing}"; gchar *filtered; /* an empty changeset would not change an empty database */ @@ -662,7 +664,8 @@ test_filter_changes (void) g_assert_cmpstr (filtered, ==, a1); g_free (filtered); - /* a changeset would change a database with the same keys but different values */ + /* a changeset would change a database with the same keys but + * different values */ filtered = call_filter_changes (a1, a2); g_assert_cmpstr (filtered, ==, a2); g_free (filtered); @@ -675,25 +678,65 @@ test_filter_changes (void) g_assert_cmpstr (filtered, ==, b2); g_free (filtered); - /* A changeset would change a database with some equal and some new values */ + /* A changeset would change a database with some equal and some new + * values */ filtered = call_filter_changes (a1, a1b2); g_assert_cmpstr (filtered, ==, b2); g_free (filtered); - /* A changeset would not change a database with some equal and some new values */ + /* A changeset would not change a database with some equal and some + * new values */ g_assert_null (call_filter_changes (a1b2, a1)); - /* Resets should count when there are values to be reset */ - filtered = call_filter_changes (a_null, reset); - g_assert_cmpstr (filtered, ==, reset); + /* A root reset has an effect on a database with values */ + filtered = call_filter_changes (a1, root_reset); + g_assert_cmpstr (filtered, ==, root_reset); g_free (filtered); - filtered = call_filter_changes (a1b2, reset); - g_assert_cmpstr (filtered, ==, reset); + + filtered = call_filter_changes (a1b2, root_reset); + g_assert_cmpstr (filtered, ==, root_reset); + g_free (filtered); + + /* A root reset would have no effect on an empty database */ + filtered = call_filter_changes (empty, root_reset); + g_assert_cmpstr (filtered, ==, NULL); + g_free (filtered); + + /* A key reset would have no effect on an empty database */ + filtered = call_filter_changes (empty, key_reset); + g_assert_cmpstr (filtered, ==, NULL); + g_free (filtered); + + /* A key reset would have no effect on a database with other keys */ + filtered = call_filter_changes (b2, key_reset); + g_assert_cmpstr (filtered, ==, NULL); + g_free (filtered); + + /* A key reset would have an effect on a database containing that + * key */ + filtered = call_filter_changes (a1, key_reset); + g_assert_cmpstr (filtered, ==, key_reset); + g_free (filtered); + + filtered = call_filter_changes (a1b1, key_reset); + g_assert_cmpstr (filtered, ==, key_reset); + g_free (filtered); + + /* A partial reset would have no effect on an empty database */ + filtered = call_filter_changes (empty, partial_reset); + g_assert_cmpstr (filtered, ==, NULL); + g_free (filtered); + + /* A partial reset would have no effect on a database with other + * values */ + filtered = call_filter_changes (a1, partial_reset); + g_assert_cmpstr (filtered, ==, NULL); g_free (filtered); - /* FIXME: ideally, a reset would have no effect on an empty database */ - filtered = call_filter_changes (empty, reset); - g_assert_cmpstr (filtered, ==, reset); + /* A partial reset would have an effect on a database with some values + * under that path */ + filtered = call_filter_changes (a1r1, partial_reset); + g_assert_cmpstr (filtered, ==, partial_reset); g_free (filtered); } -- cgit v1.2.1 From b3d8cd4757ef9344941b40d89c685c7ebefe5114 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Mon, 13 Aug 2018 14:36:36 +1000 Subject: Changeset: factor out some memory management from test_filter_changes --- tests/changeset.c | 83 ++++++++++++++++++------------------------------------- 1 file changed, 27 insertions(+), 56 deletions(-) diff --git a/tests/changeset.c b/tests/changeset.c index d75e8d0..e3153e2 100644 --- a/tests/changeset.c +++ b/tests/changeset.c @@ -610,8 +610,10 @@ string_from_changeset (DConfChangeset *changeset) return string; } -static gchar* -call_filter_changes (const gchar *base_string, const gchar *changes_string) +static void +call_filter_changes (const gchar *base_string, + const gchar *changes_string, + const gchar *expected) { DConfChangeset *base, *changes, *filtered; gchar *filtered_string = NULL; @@ -625,9 +627,11 @@ call_filter_changes (const gchar *base_string, const gchar *changes_string) dconf_changeset_unref (filtered); } + g_assert_cmpstr (filtered_string, ==, expected); + dconf_changeset_unref (base); dconf_changeset_unref (changes); - return filtered_string; + g_free (filtered_string); } static void @@ -647,97 +651,64 @@ test_filter_changes (void) const gchar *key_reset = "{'/a': @mv nothing}"; const gchar *root_reset = "{'/': @mv nothing}"; const gchar *partial_reset = "{'/r/': @mv nothing}"; - gchar *filtered; /* an empty changeset would not change an empty database */ - g_assert_null (call_filter_changes (empty, empty)); + call_filter_changes (empty, empty, NULL); /* an empty changeset would not change a database with values */ - g_assert_null (call_filter_changes (a1, empty)); + call_filter_changes (a1, empty, NULL); /* a changeset would not change a database with the same values */ - g_assert_null (call_filter_changes (a1, a1)); - g_assert_null (call_filter_changes (a1b2, a1b2)); + call_filter_changes (a1, a1, NULL); + call_filter_changes (a1b2, a1b2, NULL); /* A non-empty changeset would change an empty database */ - filtered = call_filter_changes (empty, a1); - g_assert_cmpstr (filtered, ==, a1); - g_free (filtered); + call_filter_changes (empty, a1, a1); /* a changeset would change a database with the same keys but * different values */ - filtered = call_filter_changes (a1, a2); - g_assert_cmpstr (filtered, ==, a2); - g_free (filtered); - filtered = call_filter_changes (a1b1, a1b2); - g_assert_cmpstr (filtered, ==, b2); - g_free (filtered); + call_filter_changes (a1, a2, a2); + call_filter_changes (a1b1, a1b2, b2); /* A changeset would change a database with disjoint values */ - filtered = call_filter_changes (a1, b2); - g_assert_cmpstr (filtered, ==, b2); - g_free (filtered); + call_filter_changes (a1, b2, b2); /* A changeset would change a database with some equal and some new * values */ - filtered = call_filter_changes (a1, a1b2); - g_assert_cmpstr (filtered, ==, b2); - g_free (filtered); + call_filter_changes (a1, a1b2, b2); /* A changeset would not change a database with some equal and some * new values */ - g_assert_null (call_filter_changes (a1b2, a1)); + call_filter_changes (a1b2, a1, NULL); /* A root reset has an effect on a database with values */ - filtered = call_filter_changes (a1, root_reset); - g_assert_cmpstr (filtered, ==, root_reset); - g_free (filtered); - - filtered = call_filter_changes (a1b2, root_reset); - g_assert_cmpstr (filtered, ==, root_reset); - g_free (filtered); + call_filter_changes (a1, root_reset, root_reset); + call_filter_changes (a1b2, root_reset, root_reset); /* A root reset would have no effect on an empty database */ - filtered = call_filter_changes (empty, root_reset); - g_assert_cmpstr (filtered, ==, NULL); - g_free (filtered); + call_filter_changes (empty, root_reset, NULL); /* A key reset would have no effect on an empty database */ - filtered = call_filter_changes (empty, key_reset); - g_assert_cmpstr (filtered, ==, NULL); - g_free (filtered); + call_filter_changes (empty, key_reset, NULL); /* A key reset would have no effect on a database with other keys */ - filtered = call_filter_changes (b2, key_reset); - g_assert_cmpstr (filtered, ==, NULL); - g_free (filtered); + call_filter_changes (b2, key_reset, NULL); /* A key reset would have an effect on a database containing that * key */ - filtered = call_filter_changes (a1, key_reset); - g_assert_cmpstr (filtered, ==, key_reset); - g_free (filtered); - - filtered = call_filter_changes (a1b1, key_reset); - g_assert_cmpstr (filtered, ==, key_reset); - g_free (filtered); + call_filter_changes (a1, key_reset, key_reset); + call_filter_changes (a1b1, key_reset, key_reset); /* A partial reset would have no effect on an empty database */ - filtered = call_filter_changes (empty, partial_reset); - g_assert_cmpstr (filtered, ==, NULL); - g_free (filtered); + call_filter_changes (empty, partial_reset, NULL); /* A partial reset would have no effect on a database with other * values */ - filtered = call_filter_changes (a1, partial_reset); - g_assert_cmpstr (filtered, ==, NULL); - g_free (filtered); + call_filter_changes (a1, partial_reset, NULL); /* A partial reset would have an effect on a database with some values * under that path */ - filtered = call_filter_changes (a1r1, partial_reset); - g_assert_cmpstr (filtered, ==, partial_reset); - g_free (filtered); + call_filter_changes (a1r1, partial_reset, partial_reset); } int -- cgit v1.2.1 From 4b9dc6ff18edcd07efb7d9638fdfae9db68d8341 Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Sun, 14 Jul 2019 13:15:59 +1000 Subject: Service: add tests exercising new functionality to avoid redundant writes --- tests/writer.c | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) diff --git a/tests/writer.c b/tests/writer.c index 955ba91..e5a1d61 100644 --- a/tests/writer.c +++ b/tests/writer.c @@ -179,6 +179,139 @@ test_writer_begin_corrupt_file (Fixture *fixture, } } +/** + * Test that committing a write operation when no writes have been queued + * does not result in a database write. + */ +static void test_writer_commit_no_change (Fixture *fixture, + gconstpointer test_data) +{ + const char *db_name = "nonexistent"; + g_autoptr(DConfWriter) writer = NULL; + DConfWriterClass *writer_class; + gboolean retval; + g_autoptr(GError) local_error = NULL; + g_autofree gchar *db_filename = g_build_filename (fixture->dconf_dir, db_name, NULL); + + /* Create a writer. */ + writer = DCONF_WRITER (dconf_writer_new (DCONF_TYPE_WRITER, db_name)); + g_assert_nonnull (writer); + writer_class = DCONF_WRITER_GET_CLASS (writer); + + /* Check the database doesn’t exist. */ + g_assert_false (g_file_test (db_filename, G_FILE_TEST_EXISTS)); + + /* Begin transaction */ + retval = writer_class->begin (writer, &local_error); + g_assert_no_error (local_error); + g_assert_true (retval); + + /* Commit transaction */ + retval = writer_class->commit (writer, &local_error); + g_assert_no_error (local_error); + g_assert_true (retval); + + /* Check the database still doesn’t exist. */ + g_assert_false (g_file_test (db_filename, G_FILE_TEST_EXISTS)); + + /* End transaction */ + writer_class->end (writer); +} + +/** + * Test that committing a write operation when writes that would not change + * the database have been queued does not result in a database write. + */ +static void test_writer_commit_empty_changes (Fixture *fixture, + gconstpointer test_data) +{ + const char *db_name = "nonexistent"; + g_autoptr(DConfWriter) writer = NULL; + DConfWriterClass *writer_class; + gboolean retval; + g_autoptr(GError) local_error = NULL; + g_autofree gchar *db_filename = g_build_filename (fixture->dconf_dir, db_name, NULL); + + /* Create a writer. */ + writer = DCONF_WRITER (dconf_writer_new (DCONF_TYPE_WRITER, db_name)); + g_assert_nonnull (writer); + writer_class = DCONF_WRITER_GET_CLASS (writer); + + /* Check the database doesn’t exist. */ + g_assert_false (g_file_test (db_filename, G_FILE_TEST_EXISTS)); + + /* Begin transaction */ + retval = writer_class->begin (writer, &local_error); + g_assert_no_error (local_error); + g_assert_true (retval); + + /* Make a redundant/empty change to the database */ + DConfChangeset *changes = dconf_changeset_new(); + writer_class->change (writer, changes, NULL); + g_assert_no_error (local_error); + g_assert_true (retval); + + /* Commit transaction */ + retval = writer_class->commit (writer, &local_error); + g_assert_no_error (local_error); + g_assert_true (retval); + + /* Check the database still doesn't exist */ + g_assert_false (g_file_test (db_filename, G_FILE_TEST_EXISTS)); + + /* End transaction */ + writer_class->end (writer); +} + +/** + * Test that committing a write operation when writes that would change + * the database have been queued does result in a database write. + */ +static void test_writer_commit_real_changes (Fixture *fixture, + gconstpointer test_data) +{ + const char *db_name = "nonexistent"; + g_autoptr(DConfWriter) writer = NULL; + DConfWriterClass *writer_class; + gboolean retval; + g_autoptr(GError) local_error = NULL; + g_autofree gchar *db_filename = g_build_filename (fixture->dconf_dir, db_name, NULL); + + /* Create a writer. */ + writer = DCONF_WRITER (dconf_writer_new (DCONF_TYPE_WRITER, db_name)); + g_assert_nonnull (writer); + writer_class = DCONF_WRITER_GET_CLASS (writer); + + /* Check the database doesn’t exist. */ + g_assert_false (g_file_test (db_filename, G_FILE_TEST_EXISTS)); + + /* Begin transaction */ + retval = writer_class->begin (writer, &local_error); + g_assert_no_error (local_error); + g_assert_true (retval); + + /* Make a real change to the database */ + DConfChangeset *changes = dconf_changeset_new(); + dconf_changeset_set(changes, "/key", g_variant_new ("(s)", "value")); + writer_class->change (writer, changes, NULL); + g_assert_no_error (local_error); + g_assert_true (retval); + + /* Commit transaction */ + retval = writer_class->commit (writer, &local_error); + g_assert_no_error (local_error); + g_assert_true (retval); + + /* Check the database now exists */ + g_assert_true (g_file_test (db_filename, G_FILE_TEST_EXISTS)); + + /* End transaction */ + writer_class->end (writer); + + /* Clean up. */ + g_assert_cmpint (g_unlink (db_filename), ==, 0); +} + int main (int argc, char **argv) { @@ -221,6 +354,12 @@ main (int argc, char **argv) test_writer_begin_corrupt_file, tear_down); g_test_add ("/writer/begin/corrupt-file/2", Fixture, &corrupt_file_data2, set_up, test_writer_begin_corrupt_file, tear_down); + g_test_add ("/writer/commit/redundant_change/0", Fixture, NULL, set_up, + test_writer_commit_no_change, tear_down); + g_test_add ("/writer/commit/redundant_change/1", Fixture, NULL, set_up, + test_writer_commit_empty_changes, tear_down); + g_test_add ("/writer/commit/redundant_change/2", Fixture, NULL, set_up, + test_writer_commit_real_changes, tear_down); retval = g_test_run (); -- cgit v1.2.1 From f3104f75f945bd3418e5cf59fcc7c5044c071f2b Mon Sep 17 00:00:00 2001 From: Daniel Playfair Cal Date: Mon, 11 Nov 2019 12:07:08 +1100 Subject: Service: avoid redundant writes even after other non redundant writes have succeeded --- service/dconf-writer.c | 2 ++ tests/test-dconf.py | 1 - tests/writer.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 69 insertions(+), 2 deletions(-) diff --git a/service/dconf-writer.c b/service/dconf-writer.c index 8b59019..4d054c8 100644 --- a/service/dconf-writer.c +++ b/service/dconf-writer.c @@ -183,6 +183,8 @@ dconf_writer_real_commit (DConfWriter *writer, close (invalidate_fd); } + writer->priv->need_write = FALSE; + if (writer->priv->commited_values) dconf_changeset_unref (writer->priv->commited_values); writer->priv->commited_values = writer->priv->uncommited_values; diff --git a/tests/test-dconf.py b/tests/test-dconf.py index 6cd80a8..5e65884 100755 --- a/tests/test-dconf.py +++ b/tests/test-dconf.py @@ -516,7 +516,6 @@ class DBusTest(unittest.TestCase): # Lexicographically last value should win: self.assertEqual(dconf_read('/org/file'), '99') - @unittest.expectedFailure def test_redundant_disk_writes(self): """Redundant disk writes are avoided. diff --git a/tests/writer.c b/tests/writer.c index e5a1d61..df33fc4 100644 --- a/tests/writer.c +++ b/tests/writer.c @@ -45,6 +45,29 @@ assert_n_warnings (guint expected_n_warnings) n_warnings = 0; } +static guint64 +get_file_mtime_us (char *filename) +{ + GFile *file = g_file_new_for_path (filename); + GError *error = NULL; + GFileInfo *info = g_file_query_info ( + file, + "time::*", + G_FILE_QUERY_INFO_NONE, + NULL, + &error); + if (!info) + { + printf ("failed with error %i: %s\n", error->code, error->message); + exit (1); + } + + guint64 mtime_us = + g_file_info_get_attribute_uint64 (info, G_FILE_ATTRIBUTE_TIME_MODIFIED) * 1000000 + + g_file_info_get_attribute_uint32 (info, G_FILE_ATTRIBUTE_TIME_MODIFIED_USEC); + return mtime_us; +} + typedef struct { gchar *dconf_dir; /* (owned) */ @@ -273,8 +296,10 @@ static void test_writer_commit_real_changes (Fixture *fixture, const char *db_name = "nonexistent"; g_autoptr(DConfWriter) writer = NULL; DConfWriterClass *writer_class; + DConfChangeset *changes; gboolean retval; g_autoptr(GError) local_error = NULL; + guint64 db_mtime_us; g_autofree gchar *db_filename = g_build_filename (fixture->dconf_dir, db_name, NULL); /* Create a writer. */ @@ -291,7 +316,7 @@ static void test_writer_commit_real_changes (Fixture *fixture, g_assert_true (retval); /* Make a real change to the database */ - DConfChangeset *changes = dconf_changeset_new(); + changes = dconf_changeset_new(); dconf_changeset_set(changes, "/key", g_variant_new ("(s)", "value")); writer_class->change (writer, changes, NULL); g_assert_no_error (local_error); @@ -304,6 +329,47 @@ static void test_writer_commit_real_changes (Fixture *fixture, /* Check the database now exists */ g_assert_true (g_file_test (db_filename, G_FILE_TEST_EXISTS)); + db_mtime_us = get_file_mtime_us (db_filename); + + /* End transaction */ + writer_class->end (writer); + + /* Begin a second transaction */ + retval = writer_class->begin (writer, &local_error); + g_assert_no_error (local_error); + g_assert_true (retval); + + /* Make a redundant/empty change to the database */ + changes = dconf_changeset_new(); + writer_class->change (writer, changes, NULL); + g_assert_no_error (local_error); + g_assert_true (retval); + + /* Commit transaction */ + retval = writer_class->commit (writer, &local_error); + g_assert_no_error (local_error); + g_assert_true (retval); + + /* End transaction */ + writer_class->end (writer); + + /* Check that no extra write was done (even afer committing a real change) */ + g_assert_cmpuint (db_mtime_us, ==, get_file_mtime_us (db_filename)); + db_mtime_us = get_file_mtime_us (db_filename); + + /* Begin a third transaction */ + retval = writer_class->begin (writer, &local_error); + g_assert_no_error (local_error); + g_assert_true (retval); + + /* Commit transaction (with no changes at all) */ + retval = writer_class->commit (writer, &local_error); + g_assert_no_error (local_error); + g_assert_true (retval); + + /* Check that no extra write was done (even afer committing a real change) */ + g_assert_cmpuint (db_mtime_us, ==, get_file_mtime_us (db_filename)); + db_mtime_us = get_file_mtime_us (db_filename); /* End transaction */ writer_class->end (writer); -- cgit v1.2.1