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

[André Auzi]

Transactions for bookmarks

Seventh step for merge of fix-1179200

Here the database transactions are added in bookmarks addition to improve the processing speed.

Import bookmarks time is significantly reduced: my 870 bookmark items formerly imported in 9mn40s are now imported in approx 2s.

I've reconsidered my position about queing database operations and processing them in idle time.

The fact is that those queing where originally motivated by my observation of multiple updates consecutive of drag-n-drop operations in the bookmarks panel.

Another fact is that they are significantly modified by the way I've reworked the bookmark panel to allow multiple selection drag-n-drop (this is coming in next steps and bug-894143).

I'll see, when it comes to the actual merge of this bug if this design is still necessary.

I apologize for the delay on processing the former remarks, I was kind of busy afk after summer time.

[982c80311320c1b (alexander-wilms)]

Importing ~70 bookmarks was reasonably quick

[982c80311320c1b (alexander-wilms)]

Oh, just noticed the merge failed due to a conflict. Maybe the slow-down is only noticable with a lot more bookmarks

[Cris Dywan (kalikiana)]

> with_transaction = midori_bookmarks_db_begin_transaction (bookmarks->db);
> …
> if (with_transaction)
> midori_bookmarks_db_end_transaction (bookmarks->db, TRUE);

This is wrong. If the transaction fails the function should properly abort rather than work-around and potentially hide the actual error.

> g_print ("midori_bookmarks_db_idle: %d DB operation(s) in %lu micro-seconds\n",

This should be wrapped in midori_debug ("bookmarks")

midori_bookmarks_db_idle_func:
Shouldn't the UI change be immediate even if writing to the database takes more time?
Can it be done with a single queue instead of 3? It's quite complex and doesn't look like it will be very easy to maintain. Would it make sense to have a query queue API in the database to make things more manageable and less specific to only one piece in the codebase?

review: Needs Fixing

[André Auzi (aauzi)]

> > with_transaction = midori_bookmarks_db_begin_transaction (bookmarks->db);
> > …
> > if (with_transaction)
> > midori_bookmarks_db_end_transaction (bookmarks->db, TRUE);
>
> This is wrong. If the transaction fails the function should properly abort
> rather than work-around and potentially hide the actual error.
>

Oups, I agree.

> > g_print ("midori_bookmarks_db_idle: %d DB operation(s) in %lu micro-
> seconds\n",
>
> This should be wrapped in midori_debug ("bookmarks")
>

OK again

> midori_bookmarks_db_idle_func:
> Shouldn't the UI change be immediate even if writing to the database takes
> more time?

I don't think so... A single UI change can induce loads of database operations, think about a recursive delete or a multi-selection move

you only get benefit from the transaction when packing multiple database operations together in the same transaction.

Doing this in the idle func let the API clients freely work on the items not having such constraint.

As soon as the UI operation is completely done, the database operation can be comitted.

> Can it be done with a single queue instead of 3? It's quite complex and
> doesn't look like it will be very easy to maintain. Would it make sense to
> have a query queue API in the database to make things more manageable and less
> specific to only one piece in the codebase?

I agree, it's complex enough.

My goal here was to reuse the actual database operation as they were and avoid un-necessay multiple updates, I guess this could be simplified by the creation of a "batch" queue which encodes the database operation into a struct.

struct _db_operation {
  enum _db_opcode {
    DB_CREATE,
    DB_UPDATE,
    DB_DELETE
  } opcode;
  gpointer item;
}

Alternatively, I remember that pfor mentioned he prototyped something based on the collection of database statements into a string, this may be a viable alternative but would require to rewrite the sqlite statement functions and would prevent from multiple updates optimization.

It must be kept in mind though that dragndrop feature can introduce a lot of traffic of item updates (for instance for reparenting and positions) that may justify this level of caching.

These are different options, my choice wasn't the more simple but aimed at minimizing the database traffic while reusing the existing statement code, I agree that keeping things simple should also be a considered.

If you have a preferred alternative I'll give it a try.

[Cris Dywan (kalikiana)]

Why does the queue need to care what kind of statement is executes? At the least adding all in one would simplify the code quite a lot.

I wouldn't block on whether this becomes a shared database API necessarily.

review: Needs Information

[Cris Dywan (kalikiana)]

Looks sensible.

Note: I'm giving primarily technical review not real life stress testing, somebody else *must* try this, I'm not eager to get more regressions in.

review: Approve (code-review)

[Jeremy Wootten (jeremywootten)]

I do not feel qualified to review Midori branches, other than to compile
and run them, at the moment I am afraid.

On 16 April 2014 02:25, Daniel Fore <email address hidden> wrote:

> You have been requested to review the proposed merge of
> lp:~aauzi/midori/fix-1179200-7 into lp:midori.
>
> For more details, see:
> https://code.launchpad.net/~aauzi/midori/fix-1179200-7/+merge/200058
>
> Seventh step for merge of fix-1179200
>
> Here the database transactions are added in bookmarks addition to improve
> the processing speed.
>
> Import bookmarks time is significantly reduced: my 870 bookmark items
> formerly imported in 9mn40s are now imported in approx 2s.
>
> I've reconsidered my position about queing database operations and
> processing them in idle time.
>
> The fact is that those queing where originally motivated by my observation
> of multiple updates consecutive of drag-n-drop operations in the bookmarks
> panel.
>
> Another fact is that they are significantly modified by the way I've
> reworked the bookmark panel to allow multiple selection drag-n-drop (this
> is coming in next steps and bug-894143).
>
> I'll see, when it comes to the actual merge of this bug if this design is
> still necessary.
>
> I apologize for the delay on processing the former remarks, I was kind of
> busy afk after summer time.
>
>
> --
> https://code.launchpad.net/~aauzi/midori/fix-1179200-7/+merge/200058
> Your team Midori Devs is requested to review the proposed merge of
> lp:~aauzi/midori/fix-1179200-7 into lp:midori.
>
> === modified file 'midori/midori-bookmarks-db.c'
> --- midori/midori-bookmarks-db.c 2013-11-08 11:40:47 +0000
> +++ midori/midori-bookmarks-db.c 2013-12-26 10:29:57 +0000
> @@ -356,6 +356,45 @@
> }
>
> /**
> + * midori_bookmarks_db_begin_transaction:
> + * @db: the removed #KatzeItem
> + *
> + * Internal function that starts an SQL transaction.
> + **/
> +static void
> +midori_bookmarks_db_begin_transaction (MidoriBookmarksDb* bookmarks)
> +{
> + char* errmsg = NULL;
> +
> + if (sqlite3_exec (bookmarks->db, "BEGIN TRANSACTION;", NULL, NULL,
> &errmsg) != SQLITE_OK)
> + {
> + g_printerr (_("Failed to begin transaction: %s\n"), errmsg);
> + sqlite3_free (errmsg);
> + }
> +}
> +
> +/**
> + * midori_bookmarks_db_end_transaction:
> + * @db: the removed #KatzeItem
> + * @commit : boolean
> + *
> + * Internal function that ends an SQL transaction.
> + * If @commit is %TRUE, the transaction is ended by a COMMIT.
> + * It is ended by a ROLLBACK otherwise.
> + **/
> +static void
> +midori_bookmarks_db_end_transaction (MidoriBookmarksDb* bookmarks)
> +{
> + char* errmsg = NULL;
> +
> + if (sqlite3_exec (bookmarks->db, "COMMIT;", NULL, NULL, &errmsg) !=
> SQLITE_OK)
> + {
> + g_printerr (_("Failed to end transaction: %s\n"), errmsg);
> + sqlite3_free (errmsg);
> + }
> +}
> +
> +/**
> * midori_bookmarks_db_add_item_recursive:
> * @item: the removed #KatzeItem
> * @bookmarks : the main bookmarks array
> @@ -629,11 +668,26 @@
> void
> midori_bookmarks_db_add_item (MidoriBookmarksDb* bookmarks, KatzeItem*
> item)
> {
> + GTimer *timer;
> + gint count;
> + gulong micr...

[Cody Garver (codygarver)]

Jeremy, I think it only emailed you because you were on a team that's on the Midori team. You don't have to review anything here.

[RabbitBot (rabbitbot-a)]

Attempt to merge into lp:midori failed due to conflicts:

text conflict in midori/midori-bookmarks-db.c

Unmerged revisions

6526. By André Auzi on 2013-12-26

merge lp:~aauzi/midori/fix-1179200-7

6525. By André Auzi on 2013-12-26

improve bookmarks import performance using db transactions

=== modified file 'midori/midori-bookmarks-db.c'
--- midori/midori-bookmarks-db.c	2013-11-08 11:40:47 +0000
+++ midori/midori-bookmarks-db.c	2013-12-26 10:29:57 +0000
@@ -356,6 +356,45 @@
 }
 
 /**
+ * midori_bookmarks_db_begin_transaction:
+ * @db: the removed #KatzeItem
+ *
+ * Internal function that starts an SQL transaction.
+ **/
+static void
+midori_bookmarks_db_begin_transaction (MidoriBookmarksDb* bookmarks)
+{
+    char* errmsg = NULL;
+
+    if (sqlite3_exec (bookmarks->db, "BEGIN TRANSACTION;", NULL, NULL, &errmsg) != SQLITE_OK)
+    {
+        g_printerr (_("Failed to begin transaction: %s\n"), errmsg);
+        sqlite3_free (errmsg);
+    }
+}
+
+/**
+ * midori_bookmarks_db_end_transaction:
+ * @db: the removed #KatzeItem
+ * @commit : boolean
+ *
+ * Internal function that ends an SQL transaction.
+ * If @commit is %TRUE, the transaction is ended by a COMMIT.
+ * It is ended by a ROLLBACK otherwise.
+ **/
+static void
+midori_bookmarks_db_end_transaction (MidoriBookmarksDb* bookmarks)
+{
+    char* errmsg = NULL;
+
+    if (sqlite3_exec (bookmarks->db, "COMMIT;", NULL, NULL, &errmsg) != SQLITE_OK)
+    {
+        g_printerr (_("Failed to end transaction: %s\n"), errmsg);
+        sqlite3_free (errmsg);
+    }
+}
+
+/**
  * midori_bookmarks_db_add_item_recursive:
  * @item: the removed #KatzeItem
  * @bookmarks : the main bookmarks array
@@ -629,11 +668,26 @@
 void
 midori_bookmarks_db_add_item (MidoriBookmarksDb* bookmarks, KatzeItem* item)
 {
+    GTimer *timer;
+    gint count;
+    gulong microseconds;
+
     g_return_if_fail (IS_MIDORI_BOOKMARKS_DB (bookmarks));
     g_return_if_fail (KATZE_IS_ITEM (item));
     g_return_if_fail (NULL == katze_item_get_meta_string (item, "id"));
 
-    midori_bookmarks_db_add_item_recursive (bookmarks, item);
+    midori_bookmarks_db_begin_transaction (bookmarks);
+
+    timer = g_timer_new();
+    count = midori_bookmarks_db_add_item_recursive (bookmarks, item);
+    g_timer_elapsed (timer, &microseconds);
+    if (midori_debug ("bookmarks"))
+    {
+	g_print ("midori_bookmarks_db_add_item: %d item(s) added in %lu micro-seconds\n",
+		 count, microseconds);
+    }
+
+    midori_bookmarks_db_end_transaction (bookmarks);
 
     katze_array_add_item (KATZE_ARRAY (bookmarks), item);
 }
@@ -713,7 +767,7 @@
     
     if (error != NULL)
     {
-	*errmsg = g_strdup (error->message);
+        *errmsg = g_strdup (error->message);
         g_error_free (error);
         return NULL;
     }