[PATCH 08/17] tracker: rework item cache

[lionel g landwerlin linux intel com]

From: Lionel Landwerlin <lionel g landwerlin linux intel com>

The current item cache leads to crashes because we do not invalidate
items once a source has been removed.

Signed-off-by: Lionel Landwerlin <lionel g landwerlin linux intel com>
---
 src/media/tracker/grl-tracker-api.c   |    6 +-
 src/media/tracker/grl-tracker-cache.c |  149 ++++++++++++++++++++++++++-------
 src/media/tracker/grl-tracker-cache.h |   18 ++--
 src/media/tracker/grl-tracker-notif.c |   10 +-
 src/media/tracker/grl-tracker-priv.h  |    2 +-
 src/media/tracker/grl-tracker.c       |   21 +++--
 6 files changed, 150 insertions(+), 56 deletions(-)

diff --git a/src/media/tracker/grl-tracker-api.c b/src/media/tracker/grl-tracker-api.c
index bb716ea..4b8530e 100644
--- a/src/media/tracker/grl-tracker-api.c
+++ b/src/media/tracker/grl-tracker-api.c
@@ -177,9 +177,9 @@ fill_grilo_media_from_sparql (GrlTrackerSource    *source,
   case G_TYPE_STRING:
     /* Cache the source associated to this result. */
     if (assoc->grl_key == GRL_METADATA_KEY_ID) {
-      grl_tracker_item_cache_add_item (grl_tracker_item_cache,
-                                       tracker_sparql_cursor_get_integer (cursor, column),
-                                       source);
+      grl_tracker_cache_add_item (grl_tracker_item_cache,
+                                  tracker_sparql_cursor_get_integer (cursor, column),
+                                  source);
     }
     val.str_val = tracker_sparql_cursor_get_string (cursor, column, NULL);
     if (val.str_val != NULL)
diff --git a/src/media/tracker/grl-tracker-cache.c b/src/media/tracker/grl-tracker-cache.c
index b0577bd..e5801e7 100644
--- a/src/media/tracker/grl-tracker-cache.c
+++ b/src/media/tracker/grl-tracker-cache.c
@@ -26,72 +26,163 @@
 
 #include "grl-tracker-cache.h"
 
-/* TODO: is it worth to have an LRU ? */
-struct _GrlTrackerItemCache {
+typedef struct {
+  GrlTrackerSource *source;
+
+  GHashTable *id_table;
+} GrlTrackerCacheSource;
+
+struct _GrlTrackerCache {
   gsize size_limit;
   gsize size_current;
 
-  GHashTable *table;
-  GList *list;
+  GHashTable *id_table;
+  GHashTable *source_table;
+  GList      *id_list;
 };
 
-GrlTrackerItemCache *
-grl_tracker_item_cache_new (gsize size)
+static GrlTrackerCacheSource *
+grl_tracker_cache_source_new (GrlTrackerSource *source)
+{
+  GrlTrackerCacheSource *csource = g_slice_new0 (GrlTrackerCacheSource);
+
+  csource->source = source;
+  csource->id_table = g_hash_table_new (g_direct_hash, g_direct_equal);
+
+  return csource;
+}
+
+static void
+grl_tracker_cache_source_free (GrlTrackerCacheSource *csource)
 {
-  GrlTrackerItemCache *cache;
+  g_hash_table_destroy (csource->id_table);
+
+  g_slice_free (GrlTrackerCacheSource, csource);
+}
+
+/**/
+
+GrlTrackerCache *
+grl_tracker_cache_new (gsize size)
+{
+  GrlTrackerCache *cache;
 
   g_return_val_if_fail (size > 0, NULL);
 
-  cache = g_slice_new0 (GrlTrackerItemCache);
+  cache = g_slice_new0 (GrlTrackerCache);
 
   if (!cache)
     return NULL;
 
-  cache->size_limit = size;
-  cache->table = g_hash_table_new (g_direct_hash, g_direct_equal);
+  cache->size_limit   = size;
+  cache->id_table     = g_hash_table_new (g_direct_hash, g_direct_equal);
+  cache->source_table = g_hash_table_new (g_direct_hash, g_direct_equal);
 
   return cache;
 }
 
 void
-grl_tracker_item_cache_free (GrlTrackerItemCache *cache)
+grl_tracker_cache_free (GrlTrackerCache *cache)
 {
+  GHashTableIter iter;
+  gpointer key, value;
+
   g_return_if_fail (cache != NULL);
 
-  g_list_free (cache->list);
-  g_hash_table_destroy (cache->table);
+  g_hash_table_iter_init (&iter, cache->source_table);
+  while (g_hash_table_iter_next (&iter, &key, &value)) {
+    grl_tracker_cache_del_source (cache, key);
+  }
 
-  g_slice_free (GrlTrackerItemCache, cache);
+  if (cache->id_list) {
+    g_warning ("Memleak detected");
+    g_list_free (cache->id_list);
+  }
+  g_hash_table_destroy (cache->id_table);
+  g_hash_table_destroy (cache->source_table);
+
+  g_slice_free (GrlTrackerCache, cache);
 }
 
 void
-grl_tracker_item_cache_add_item (GrlTrackerItemCache *cache,
-				 guint id,
-				 GrlTrackerSource *source)
+grl_tracker_cache_add_item (GrlTrackerCache *cache,
+                            guint id,
+                            GrlTrackerSource *source)
 {
-  GList *last;
+  GList *lid;
+  GrlTrackerCacheSource *csource;
 
   g_return_if_fail (cache != NULL);
 
-  if (g_hash_table_lookup (cache->table, GSIZE_TO_POINTER (id)) != NULL)
-    return;
+  if (g_hash_table_lookup (cache->id_table, GSIZE_TO_POINTER (id)) != NULL)
+    return; /* TODO: is it worth to have an LRU ? */
+
+  csource = g_hash_table_lookup (cache->source_table, source);
+
+  if (!csource) {
+    csource = grl_tracker_cache_source_new (source);
+    g_hash_table_insert (cache->source_table, source, csource);
+  }
 
   if (cache->size_current >= cache->size_limit) {
-    last = g_list_last (cache->list); /* TODO: optimize that ! */
-    g_hash_table_remove (cache->table, last->data);
-    cache->list = g_list_remove_link (cache->list, last);
+    lid = g_list_last (cache->id_list); /* TODO: optimize that ! */
+    g_hash_table_remove (cache->id_table, lid->data);
+    cache->id_list = g_list_remove_link (cache->id_list, lid);
+
+    lid->data = GSIZE_TO_POINTER (id);
+    lid->next = cache->id_list;
+    cache->id_list->prev = lid;
+    cache->id_list = lid;
+  } else {
+    cache->id_list = g_list_prepend (cache->id_list, GSIZE_TO_POINTER (id));
+    cache->size_current++;
+  }
+
+  g_hash_table_insert (cache->id_table, GSIZE_TO_POINTER (id), csource);
+  g_hash_table_insert (csource->id_table, GSIZE_TO_POINTER (id),
+                       cache->id_list);
+}
+
+void
+grl_tracker_cache_del_source (GrlTrackerCache *cache,
+                              GrlTrackerSource *source)
+{
+  GrlTrackerCacheSource *csource;
+  GHashTableIter iter;
+  gpointer key, value;
+
+  g_return_if_fail (cache != NULL);
+  g_return_if_fail (source != NULL);
+
+  csource = g_hash_table_lookup (cache->source_table, source);
+
+  if (!csource)
+    return;
+
+  g_hash_table_iter_init (&iter, csource->id_table);
+
+  while (g_hash_table_iter_next (&iter, &key, &value)) {
+    g_hash_table_remove (cache->id_table, key);
+    cache->id_list = g_list_delete_link (cache->id_list, value);
   }
 
-  cache->list = g_list_prepend (cache->list, GSIZE_TO_POINTER (id));
-  g_hash_table_insert (cache->table, GSIZE_TO_POINTER (id), source);
-  cache->size_current++;
+  g_hash_table_remove (cache->source_table, source);
+  grl_tracker_cache_source_free (csource);
 }
 
 GrlTrackerSource *
-grl_tracker_item_cache_get_source (GrlTrackerItemCache *cache, guint id)
+grl_tracker_cache_get_source (GrlTrackerCache *cache, guint id)
 {
+  GrlTrackerCacheSource *csource;
+
   g_return_val_if_fail (cache != NULL, NULL);
 
-  return (GrlTrackerSource *) g_hash_table_lookup (cache->table,
-						   GSIZE_TO_POINTER (id));
+  csource = (GrlTrackerCacheSource *) g_hash_table_lookup (cache->id_table,
+                                                      GSIZE_TO_POINTER (id));
+
+  if (csource) {
+    return csource->source;
+  }
+
+  return NULL;
 }
diff --git a/src/media/tracker/grl-tracker-cache.h b/src/media/tracker/grl-tracker-cache.h
index 5761cf2..ad07865 100644
--- a/src/media/tracker/grl-tracker-cache.h
+++ b/src/media/tracker/grl-tracker-cache.h
@@ -27,17 +27,19 @@
 
 #include "grl-tracker.h"
 
-typedef struct _GrlTrackerItemCache GrlTrackerItemCache;
+typedef struct _GrlTrackerCache GrlTrackerCache;
 
-GrlTrackerItemCache *grl_tracker_item_cache_new (gsize size);
+GrlTrackerCache *grl_tracker_cache_new (gsize size);
 
-void grl_tracker_item_cache_free (GrlTrackerItemCache *cache);
+void grl_tracker_cache_free (GrlTrackerCache *cache);
 
-void grl_tracker_item_cache_add_item (GrlTrackerItemCache *cache,
-				      guint id,
-				      GrlTrackerSource *source);
+void grl_tracker_cache_add_item (GrlTrackerCache *cache,
+                                 guint id,
+                                 GrlTrackerSource *source);
+void grl_tracker_cache_del_source (GrlTrackerCache *cache,
+                                   GrlTrackerSource *source);
 
-GrlTrackerSource *grl_tracker_item_cache_get_source (GrlTrackerItemCache *cache,
-						     guint id);
+GrlTrackerSource *grl_tracker_cache_get_source (GrlTrackerCache *cache,
+                                                guint id);
 
 #endif /* _GRL_TRACKER_CACHE_H_ */
diff --git a/src/media/tracker/grl-tracker-notif.c b/src/media/tracker/grl-tracker-notif.c
index 8a8d0db..b7eecd8 100644
--- a/src/media/tracker/grl-tracker-notif.c
+++ b/src/media/tracker/grl-tracker-notif.c
@@ -139,13 +139,13 @@ tracker_evt_update_source_del (tracker_evt_update_t *evt,
 			       GrlTrackerSource *source)
 {
   GrlTrackerSourcePriv *priv = GRL_TRACKER_SOURCE_GET_PRIVATE (source);
+
   priv->notification_ref++;
   priv->state = GRL_TRACKER_SOURCE_STATE_DELETING;
 
   evt->old_sources = g_list_append (evt->old_sources, source);
 
-  GRL_DEBUG ("Predel source p=%p name=%s id=%s count=%u",
-             source,
+  GRL_DEBUG ("Predel source p=%p name=%s id=%s count=%u", source,
              grl_metadata_source_get_name (GRL_METADATA_SOURCE (source)),
              grl_metadata_source_get_id (GRL_METADATA_SOURCE (source)),
              priv->notification_ref);
@@ -214,7 +214,7 @@ tracker_evt_update_orphan_item_cb (GObject              *object,
     GrlMedia *media;
 
     GRL_DEBUG (" \tAdding to cache id=%u", id);
-    grl_tracker_item_cache_add_item (grl_tracker_item_cache, id, source);
+    grl_tracker_cache_add_item (grl_tracker_item_cache, id, source);
 
     if (grl_tracker_source_can_notify (source)) {
       media = grl_tracker_build_grilo_media (type);
@@ -557,7 +557,7 @@ tracker_dbus_signal_cb (GDBusConnection *connection,
                                 &subject, &predicate, &object)) {
       gpointer psubject = GSIZE_TO_POINTER (subject);
       GrlTrackerSource *source =
-        grl_tracker_item_cache_get_source (grl_tracker_item_cache, subject);
+        grl_tracker_cache_get_source (grl_tracker_item_cache, subject);
 
       /* GRL_DEBUG ("\tdelete=> subject=%i", subject); */
 
@@ -574,7 +574,7 @@ tracker_dbus_signal_cb (GDBusConnection *connection,
                               &subject, &predicate, &object)) {
       gpointer psubject = GSIZE_TO_POINTER (subject);
       GrlTrackerSource *source =
-        grl_tracker_item_cache_get_source (grl_tracker_item_cache, subject);
+        grl_tracker_cache_get_source (grl_tracker_item_cache, subject);
 
       /* GRL_DEBUG ("\tinsert=> subject=%i", subject); */
 
diff --git a/src/media/tracker/grl-tracker-priv.h b/src/media/tracker/grl-tracker-priv.h
index 225c527..b6c5a18 100644
--- a/src/media/tracker/grl-tracker-priv.h
+++ b/src/media/tracker/grl-tracker-priv.h
@@ -76,7 +76,7 @@ extern TrackerSparqlConnection *grl_tracker_connection;
 extern const GrlPluginInfo *grl_tracker_plugin;
 
 /* shared data across  */
-extern GrlTrackerItemCache *grl_tracker_item_cache;
+extern GrlTrackerCache *grl_tracker_item_cache;
 extern GHashTable *grl_tracker_modified_sources;
 
 /* tracker plugin config */
diff --git a/src/media/tracker/grl-tracker.c b/src/media/tracker/grl-tracker.c
index 2ce6505..62b26a7 100644
--- a/src/media/tracker/grl-tracker.c
+++ b/src/media/tracker/grl-tracker.c
@@ -77,7 +77,7 @@ TrackerSparqlConnection *grl_tracker_connection = NULL;
 const GrlPluginInfo *grl_tracker_plugin;
 
 /* shared data across  */
-GrlTrackerItemCache *grl_tracker_item_cache;
+GrlTrackerCache *grl_tracker_item_cache;
 GHashTable *grl_tracker_modified_sources;
 
 /* tracker plugin config */
@@ -100,12 +100,12 @@ grl_tracker_add_source (GrlTrackerSource *source)
   }
   if (priv->notification_ref == 0) {
     g_hash_table_remove (grl_tracker_modified_sources,
-			 grl_metadata_source_get_id (GRL_METADATA_SOURCE (source)));
+                         grl_metadata_source_get_id (GRL_METADATA_SOURCE (source)));
     priv->state = GRL_TRACKER_SOURCE_STATE_RUNNING;
     grl_plugin_registry_register_source (grl_plugin_registry_get_default (),
-					 grl_tracker_plugin,
-					 GRL_MEDIA_PLUGIN (source),
-					 NULL);
+                                         grl_tracker_plugin,
+                                         GRL_MEDIA_PLUGIN (source),
+                                         NULL);
   }
 }
 
@@ -122,11 +122,12 @@ grl_tracker_del_source (GrlTrackerSource *source)
   }
   if (priv->notification_ref == 0) {
     g_hash_table_remove (grl_tracker_modified_sources,
-			 grl_metadata_source_get_id (GRL_METADATA_SOURCE (source)));
+                         grl_metadata_source_get_id (GRL_METADATA_SOURCE (source)));
+    grl_tracker_cache_del_source (grl_tracker_item_cache, source);
     priv->state = GRL_TRACKER_SOURCE_STATE_DELETED;
     grl_plugin_registry_unregister_source (grl_plugin_registry_get_default (),
-					   GRL_MEDIA_PLUGIN (source),
-					   NULL);
+                                           GRL_MEDIA_PLUGIN (source),
+                                           NULL);
   }
 }
 
@@ -153,7 +154,7 @@ grl_tracker_source_find (const gchar *id)
     return (GrlTrackerSource *) source;
 
   return (GrlTrackerSource *) g_hash_table_lookup (grl_tracker_modified_sources,
-						   id);
+                                                   id);
 }
 
 static void
@@ -280,7 +281,7 @@ grl_tracker_plugin_init (GrlPluginRegistry *registry,
   grl_tracker_init_requests ();
 
   grl_tracker_plugin = plugin;
-  grl_tracker_item_cache = grl_tracker_item_cache_new (TRACKER_ITEM_CACHE_SIZE);
+  grl_tracker_item_cache = grl_tracker_cache_new (TRACKER_ITEM_CACHE_SIZE);
   grl_tracker_modified_sources = g_hash_table_new (g_str_hash, g_str_equal);
 
   if (!configs) {
-- 
1.7.4.1