Merge lp:~aauzi/midori/fix-1179200-6 into lp:midori

[André Auzi]

Cache bookmark items to avoid their recreation on database reads

Sixth step for the merge of fix-1179200

This step cache all bookmark items in order to avoid their recreation on database reads.

The cache is recursively populated and cleared for folders addition or removals.

Thanks tp this change, items are consistent in bookmark bar and panel and the following use case/sequence works.

Having a bookmark that's not in the boolùark bar, visible in the panel
Edit it from the panel and add it to the bookmark bar
It appears in the bookmark bar
Edit it from the bar and remove it from the bar
Edit it from the panel

Without the fix:
  the item appears with the bookmark bar state checked (the last state it had when it was edited in the panel)

With the fix:
  the item appears with the current bookmark bar state: unchecked

[982c80311320c1b (alexander-wilms)]

Works as expected

[Cris Dywan (kalikiana)]

The code looks sensible overall.
Did you consider if using sqlite lookup instead of hash tables could conserve memory and possibly be as fast as or faster than the hash tables? Provided handling of updates is reliably done you could also avoid overhead reading through all items.

review: Needs Information

[André Auzi (aauzi)]

> Did you consider if using sqlite lookup instead of hash tables could conserve
> memory and possibly be as fast as or faster than the hash tables? Provided
> handling of updates is reliably done you could also avoid overhead reading
> through all items.

My goal here was to limit the need of update for the different items attached to UI widgets.

With this cache mechanism I wanted to have the same items attached to the toolitems of bookmarks tool bar, the menuitem of the bookmarks menu and the treeitems of the bookmarks panel.

When the item is changed, the widgets are most often updated by the change notify signal,the update_item being provided for reparenting in the panel treeview.

I considered that doing it this way would preserve the data flow from the UI to the database, reduce the memory footprint and keep at the strict minimum the need to search items in the UI widget trees for copy/update after a modification.

I also wanted to centralize the data to be able, later, to implement Undo of massive changes like recursive deletes or imports.

Unless I'm mistaken, sqlite lookups would only partially provide these properties unless we could implement an association table of items with UI widgets.
It may be implemented with a temporary (in memory) table but would it be as straight forward as the proposed hash table? I don't know.

The avoidance of reread of items is implemented, at least for the bookmarks panel, at step 8 in branch fix-1179200-8.

[Cris Dywan (kalikiana)]

I concur. I don't have any evidence sqlite would do the trick and it might need significant research. I would think undo which is backed by sqlite on disk could be interesting, but not necessarily as part of the main table. So given the hash table approach works very well, there is no reason not to go with it now.

review: Approve

=== modified file 'midori/midori-bookmarks-db.c'
--- midori/midori-bookmarks-db.c	2013-09-17 19:34:23 +0000
+++ midori/midori-bookmarks-db.c	2013-09-18 17:39:50 +0000
@@ -39,6 +39,7 @@
     KatzeArray  parent_instance;
 
     sqlite3*    db;
+    GHashTable* all_items;
 };
 
 struct _MidoriBookmarksDbClass
@@ -97,6 +98,21 @@
 midori_bookmarks_db_remove_item_db (sqlite3*   db,
                                     KatzeItem* item);
 
+static guint
+item_hash (gconstpointer item)
+{
+    gint64 id = katze_item_get_meta_integer (KATZE_ITEM (item), "id");
+    return g_int64_hash (&id);
+}
+
+static gboolean
+item_equal (gconstpointer item_a, gconstpointer item_b)
+{
+    gint64 id_a = katze_item_get_meta_integer (KATZE_ITEM (item_a), "id");
+    gint64 id_b = katze_item_get_meta_integer (KATZE_ITEM (item_b), "id");
+    return (id_a == id_b)? TRUE : FALSE;
+}
+
 static void
 midori_bookmarks_db_class_init (MidoriBookmarksDbClass* class)
 {
@@ -131,9 +147,11 @@
 midori_bookmarks_db_init (MidoriBookmarksDb* bookmarks)
 {
     bookmarks->db = NULL;
+    bookmarks->all_items = g_hash_table_new (item_hash, item_equal);
 
     katze_item_set_meta_integer (KATZE_ITEM (bookmarks), "id", 0);
     katze_item_set_name (KATZE_ITEM (bookmarks), _("Bookmarks"));
+    g_hash_table_insert (bookmarks->all_items, bookmarks, bookmarks);
     /* g_object_ref (bookmarks); */
 }
 
@@ -147,6 +165,8 @@
         sqlite3_close (bookmarks->db);
     }
 
+    g_hash_table_unref (bookmarks->all_items);
+
     G_OBJECT_CLASS (midori_bookmarks_db_parent_class)->finalize (object);
 }
 
@@ -172,7 +192,13 @@
     }
     else
     {
-        parent = NULL;
+        KatzeItem *search = katze_item_new ();
+
+        katze_item_set_meta_integer(search, "id", parentid);
+
+        parent = KATZE_ARRAY (g_hash_table_lookup (bookmarks->all_items, search));
+
+        g_object_unref (search);
     }
 
     return parent;
@@ -216,7 +242,7 @@
 
     if (IS_MIDORI_BOOKMARKS_DB (parent))
         KATZE_ARRAY_CLASS (midori_bookmarks_db_parent_class)->add_item (parent, item);
-    else
+    else if (KATZE_IS_ARRAY (parent))
         katze_array_add_item (parent, item);
 }
 
@@ -267,7 +293,7 @@
 
     if (IS_MIDORI_BOOKMARKS_DB (parent))
         KATZE_ARRAY_CLASS (midori_bookmarks_db_parent_class)->remove_item (parent, item);
-    else
+    else if (KATZE_IS_ARRAY (parent))
         katze_array_remove_item (parent, item);
 }
 
@@ -331,6 +357,84 @@
 }
 
 /**
+ * midori_bookmarks_db_add_item_recursive:
+ * @item: the removed #KatzeItem
+ * @bookmarks : the main bookmarks array
+ *
+ * Internal function that creates memory records of the added @item.
+ * If @item is a #KatzeArray, the function recursiveley adds records
+ * of all its childs.
+ **/
+static gint
+midori_bookmarks_db_add_item_recursive (MidoriBookmarksDb* bookmarks,
+                                        KatzeItem* item)
+{
+    GList* list;
+    KatzeArray* array;
+    gint64 id = 0;
+    gint count = 0;
+    gint64 parentid = katze_item_get_meta_integer (item, "parentid");
+
+    id = midori_bookmarks_db_insert_item_db (bookmarks->db, item, parentid);
+    count++;
+
+    g_object_ref (item);
+    g_hash_table_insert (bookmarks->all_items, item, item);
+
+    if (!KATZE_IS_ARRAY (item))
+        return count;
+
+    array = KATZE_ARRAY (item);
+
+    KATZE_ARRAY_FOREACH_ITEM_L (item, array, list)
+    {
+        katze_item_set_meta_integer (item, "parentid", id);
+        count += midori_bookmarks_db_add_item_recursive (bookmarks, item);
+    }
+
+    g_list_free (list);
+    return count;
+}
+
+/**
+ * midori_bookmarks_db_remove_item_recursive:
+ * @item: the removed #KatzeItem
+ * @bookmarks : the main bookmarks array
+ *
+ * Internal function that removes memory records of the removed @item.
+ * If @item is a #KatzeArray, the function recursiveley removes records
+ * of all its childs.
+ **/
+static void
+midori_bookmarks_db_remove_item_recursive (KatzeItem*  item,
+                                           MidoriBookmarksDb* bookmarks)
+{
+    GHashTableIter hash_iter;
+    gpointer found;
+    KatzeArray* array;
+    KatzeItem* child;
+    GList* list;
+
+    if (NULL != (found = g_hash_table_lookup (bookmarks->all_items, item)))
+    {
+        g_hash_table_remove (bookmarks->all_items, found);
+        g_object_unref (found);
+    }
+
+    if (!KATZE_IS_ARRAY (item))
+        return;
+
+    array = KATZE_ARRAY (item);
+
+    KATZE_ARRAY_FOREACH_ITEM_L (child, array, list)
+    {
+        midori_bookmarks_db_remove_item_recursive (child, bookmarks);
+    }
+
+    g_list_free (list);
+}
+
+/**
  * midori_bookmarks_db_insert_item_db:
  * @db: the #sqlite3
  * @item: #KatzeItem the item to insert
@@ -531,8 +635,7 @@
     g_return_if_fail (KATZE_IS_ITEM (item));
     g_return_if_fail (NULL == katze_item_get_meta_string (item, "id"));
 
-    midori_bookmarks_db_insert_item_db (bookmarks->db, item,
-                                        katze_item_get_meta_integer (item, "parentid"));
+    midori_bookmarks_db_add_item_recursive (bookmarks, item);
 
     katze_array_add_item (KATZE_ARRAY (bookmarks), item);
 }
@@ -576,6 +679,7 @@
     g_return_if_fail (katze_item_get_meta_string (item, "id"));
     g_return_if_fail (0 != katze_item_get_meta_integer (item, "id"));
 
+    midori_bookmarks_db_remove_item_recursive (item, bookmarks);
     midori_bookmarks_db_remove_item_db (bookmarks->db, item);
 
     katze_array_remove_item (KATZE_ARRAY (bookmarks), item);
@@ -882,9 +986,6 @@
     {
         katze_item_set_meta_integer (item, "parentid", parentid);
         midori_bookmarks_db_add_item (bookmarks, item);
-        if (KATZE_IS_ARRAY (item))
-        midori_bookmarks_db_import_array (bookmarks, KATZE_ARRAY (item),
-                          katze_item_get_meta_integer(item, "id"));
     }
     g_list_free (list);
 }
@@ -929,7 +1030,16 @@
         for (i = 0; i < cols; i++)
             katze_item_set_value_from_column (stmt, i, item);
 
-        if (KATZE_ITEM_IS_FOLDER (item))
+        if (NULL != (found = g_hash_table_lookup (bookmarks->all_items, item)))
+        {
+            for (i = 0; i < cols; i++)
+                katze_item_set_value_from_column (stmt, i, found);
+
+            g_object_unref (item);
+
+            item = found;
+        }
+        else if (KATZE_ITEM_IS_FOLDER (item))
         {
             g_object_unref (item);
 
@@ -937,6 +1047,14 @@
 
             for (i = 0; i < cols; i++)
                 katze_item_set_value_from_column (stmt, i, item);
+
+            g_object_ref (item);
+            g_hash_table_insert (bookmarks->all_items, item, item);
+        }
+        else
+        {
+            g_object_ref (item);
+            g_hash_table_insert (bookmarks->all_items, item, item);
         }
 
         katze_array_add_item (array, item);