GNOME.org
 

[libdmapsharing] Significant work on reducing memory usage while building "/1/items" response

commit 6b59d1671be1463532b2aa34a47d2d6e315f72b9
Author: W. Michael Petullo <mike flyn org>
Date:   Mon Nov 8 18:14:56 2010 -0600

    Significant work on reducing memory usage while building "/1/items" response
    
    We previously simply called foreach...add_entry_to_mlcl and later
    serialized the entire structure. This has the disadvantage that the entire
    response must be in memory before libsoup sends it to the client. Now,
    we transmit portions at a time.
    Signed-off-by: W. Michael Petullo <mike flyn org>

 ChangeLog                       |    9 +++
 README-Memory                   |   27 +++++++-
 TODO                            |   17 +----
 libdmapsharing/daap-share.c     |    6 +-
 libdmapsharing/dmap-record.h    |    4 +-
 libdmapsharing/dmap-share.c     |  136 +++++++++++++++++++++++++++++++++++++--
 libdmapsharing/dmap-share.h     |    2 +-
 libdmapsharing/dmap-structure.c |   13 ++++
 libdmapsharing/dmap-structure.h |   28 ++++----
 libdmapsharing/dpap-share.c     |    7 +-
 tests/test-daap-record.c        |    6 +-
 11 files changed, 209 insertions(+), 46 deletions(-)
---
diff --git a/ChangeLog b/ChangeLog
index a170eb5..3d16b19 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+07 November 2010 W. Michael Petullo <mike flyn org>
+
+	* Significant work on an interim solution for the problem
+	of memory usage while building the "/1/items" response. We
+	previously simply called foreach...add_entry_to_mlcl and later
+	serialized the entire structure. This has the disadvantage that
+	the entire response must be in memory before libsoup sends it
+	to the client. Now, we transmit portions at a time.
+
 31 October 2010 W. Michael Petullo <mike flyn org>
 
 	* Bump version number.
diff --git a/README-Memory b/README-Memory
index 334bbc6..2474eb8 100644
--- a/README-Memory
+++ b/README-Memory
@@ -1,5 +1,7 @@
 This documents the effort to make dmapd and libdmapsharing more memory
-efficient.
+efficient. This document describes work done in both libdmapsharing and
+dmapd, because dmapd is the primary application of libdmapsharing's
+server-side functionality.
 
 I found that dmapd used a significant amount of heap space while trying to
 port dmapd to OpenWRT on a WRT160NL router with 32 Megabytes of RAM. A
@@ -37,7 +39,7 @@ efficient:
 Database implementation. This has not resulted in any improvement,
 so the hash table implementation is once again the default.
 
-2. Fixed several memory leaks after analysing with valgrind.
+2. Fixed several memory leaks after analyzing with valgrind.
 
 	Heap without client connection: 3,940,352 bytes
 
@@ -57,3 +59,24 @@ GSList of ID's.
 	Heap without client connection: 3,518,464 bytes
 	Heap after client connection and song list: 6,864,896 bytes
 	Max heap usages (massif): 8,945,760 bytes
+
+6. Modify libdmapsharing so that it responds to "/1/items" requests in
+chunks, avoiding the need to build the entire response in memory.
+
+	The effectiveness of this can be measured by observing the
+	maximum heap usage after a client has asked for a full media
+	list. Previous to this change, dmapd would use 7,811,072 bytes
+	of heap to describe a 2,232-song library. After this change,
+	dmapd's usage was 3,665,920 bytes, a very small increase over
+	the memory used by dmapd immediately after starting.
+
+7. Write a disk-based database for dmapd.
+
+	In order to minimize the memory used by a newly started dmapd,
+	I wrote a disk-based DMAPDb backend. This backend stores all
+	database data in files and avoids storing the entire database
+	in memory. To measure the effectiveness, I measured the memory
+	usage of dmapd after loading a 3,276-photograph library (note
+	that thumbnails are especially problematic). The GHashTable-based
+	database used 35,885,056 bytes of heap space. The disk-based
+	database used 4,763,648 bytes.
diff --git a/TODO b/TODO
index 0646aee..ca67a5a 100644
--- a/TODO
+++ b/TODO
@@ -1,19 +1,6 @@
 Reduce memory needed to send entire listing to client:
-	Can we predict length of mlcl?
-		dmapd-structure.c: what does this mean in the case of MLCL: guint32 size = GINT32_TO_BE (item->size);
-
-	dmap-share.c.../1/items use chunked response see daap-share.c:databases_items_xxx() / send_chunked_file()
-		collect ID's into list
-		header-complete callback, send preamble
-		chunk-complete callback (modify add_entry_to_mlcl into callback) take ID list as user_data
-			for each callback, pull an ID off list and write it
-			once list is empty, done
-	
-	SHOULD THIS STUFF BE USED IN NEW SCHEME? HOW TO HANDLE SERIALIZING MLCL?
-	eventually, get rid of?
-		dmap_structure_serialize
-		dmap_structure_serialize_node
-		dmap_structure_add
+	get bdb dmapd backend working well
+	review changes
 
 Fix DNSSD on Mac OS X
 
diff --git a/libdmapsharing/daap-share.c b/libdmapsharing/daap-share.c
index 05925fe..7207d3b 100644
--- a/libdmapsharing/daap-share.c
+++ b/libdmapsharing/daap-share.c
@@ -76,7 +76,7 @@ static void databases_items_xxx (DMAPShare *share,
 				 GHashTable *query,
 				 SoupClientContext *context);
 static struct DMAPMetaDataMap *get_meta_data_map (DMAPShare *share);
-static void add_entry_to_mlcl (gpointer id,
+static guint32 add_entry_to_mlcl (gpointer id,
 			       DMAPRecord *record,
 			       gpointer mb);
 
@@ -542,7 +542,7 @@ send_chunked_file (SoupServer *server, SoupMessage *message, DAAPRecord *record,
 	/* NOTE: cd g_free'd by chunked_message_finished(). */
 }
 
-static void
+static guint32
 add_entry_to_mlcl (gpointer id,
 		   DMAPRecord *record,
 		   gpointer _mb)
@@ -714,7 +714,7 @@ add_entry_to_mlcl (gpointer id,
 		dmap_structure_add (mlit, DMAP_CC_AEMK, mediakind);
 	}
 
-	return;
+	return ((DMAPStructureItem *) mlit->data)->size;
 }
 
 static void
diff --git a/libdmapsharing/dmap-record.h b/libdmapsharing/dmap-record.h
index 4a29329..09868e4 100644
--- a/libdmapsharing/dmap-record.h
+++ b/libdmapsharing/dmap-record.h
@@ -75,7 +75,9 @@ typedef unsigned long long bitwise;
 struct MLCL_Bits {
 	GNode *mlcl;
 	bitwise bits;
-	gpointer pointer;
+	/* FIXME: yuck, used for accumulater: */
+	gpointer user_data1;
+	gpointer user_data2;
 };
 
 typedef enum {
diff --git a/libdmapsharing/dmap-share.c b/libdmapsharing/dmap-share.c
index 69a3872..7b76180 100644
--- a/libdmapsharing/dmap-share.c
+++ b/libdmapsharing/dmap-share.c
@@ -91,6 +91,11 @@ struct DMAPSharePrivate {
 	GHashTable *session_ids;
 };
 
+struct share_bitwise_t {
+	DMAPShare *share;
+	bitwise bits;
+};
+
 static void dmap_share_init       (DMAPShare *share);
 static void dmap_share_class_init (DMAPShareClass *klass);
 
@@ -1444,6 +1449,81 @@ _dmap_share_ctrl_int (DMAPShare *share,
 	g_debug ("ctrl-int not implemented");
 }
 
+static void
+accumulate_mlcl_size (gpointer id,
+		      DMAPRecord *record,
+		      gpointer mb)
+{
+	/* Make copy and set mlcl to NULL so real MLCL does not get changed */
+	struct MLCL_Bits mb_copy = *((struct MLCL_Bits *) mb);
+	mb_copy.mlcl = dmap_structure_add (NULL, DMAP_CC_MLCL);;
+
+	*((guint *) ((struct MLCL_Bits *) mb)->user_data1) += DMAP_SHARE_GET_CLASS (((struct MLCL_Bits *) mb)->user_data2)->add_entry_to_mlcl (id, record, &mb_copy);
+
+	/* Destroy created structures as we go. */
+	dmap_structure_destroy (mb_copy.mlcl);
+}
+
+static void
+accumulate_ids (gpointer id,
+		DMAPRecord *record,
+		GSList **list)
+{
+	*list = g_slist_append (*list, id);
+}
+
+static void
+write_daap_preamble (SoupMessage *message, GNode *node)
+{
+	guint length;
+	gchar *data = dmap_structure_serialize (node, &length);
+	soup_message_body_append (message->response_body,
+				  SOUP_MEMORY_TAKE,
+				  data,
+				  length);
+	dmap_structure_destroy (node);
+}
+
+static void
+write_next_mlit (SoupMessage *message, struct share_bitwise_t *share_bitwise)
+{
+	gchar *data;
+	guint length;
+	DMAPRecord *record;
+	static GSList *id_list = NULL;
+	struct MLCL_Bits mb = {NULL,0};
+
+	if (id_list == NULL) {
+		g_debug ("Initializing ID list.");
+		dmap_db_foreach (share_bitwise->share->priv->db, (GHFunc) accumulate_ids, &id_list);
+	}
+
+	record = dmap_db_lookup_by_id (share_bitwise->share->priv->db, GPOINTER_TO_UINT (id_list->data));
+
+	mb.bits = share_bitwise->bits;
+	mb.mlcl = dmap_structure_add (NULL, DMAP_CC_MLCL);
+
+	DMAP_SHARE_GET_CLASS (share_bitwise->share)->add_entry_to_mlcl (id_list->data, record, &mb);
+	data = dmap_structure_serialize (g_node_first_child(mb.mlcl), &length);
+
+	soup_message_body_append (message->response_body,
+				 SOUP_MEMORY_TAKE,
+				 data,
+				 length);
+	g_debug ("Sending ID %u.", GPOINTER_TO_UINT (id_list->data));
+	dmap_structure_destroy (mb.mlcl);
+
+	id_list = g_slist_remove (id_list, id_list->data);
+	if (id_list == NULL) {
+		g_debug ("No more ID's, sending message complete.");
+		soup_message_body_complete (message->response_body);
+		g_free (share_bitwise);
+	}
+
+	g_object_unref (record);
+	soup_server_unpause_message (share_bitwise->share->priv->server, message);
+}
+
 void
 _dmap_share_databases (DMAPShare *share,
 		       SoupServer        *server,
@@ -1623,18 +1703,64 @@ _dmap_share_databases (DMAPShare *share,
 		dmap_structure_add (adbs, DMAP_CC_MUTY, 0);
 		dmap_structure_add (adbs, DMAP_CC_MTCO, (gint32) num_songs);
 		dmap_structure_add (adbs, DMAP_CC_MRCO, (gint32) num_songs);
-		mb.mlcl = dmap_structure_add (adbs, DMAP_CC_MLCL);
 
 		if (record_query) {
+			/* NOTE: still uses old technique: */
+			mb.mlcl = dmap_structure_add (adbs, DMAP_CC_MLCL); // Was shared with else before
 			g_hash_table_foreach (records, (GHFunc) DMAP_SHARE_GET_CLASS (share)->add_entry_to_mlcl, &mb);
 			g_hash_table_destroy (records);
+			_dmap_share_message_set_from_dmap_structure (share, message, adbs); // Was shared with else before
+			dmap_structure_destroy (adbs); // Was shared with else before
 		} else {
-			dmap_db_foreach (share->priv->db, (GHFunc) DMAP_SHARE_GET_CLASS (share)->add_entry_to_mlcl, &mb);
+			/* NOTE:
+			 * We previously simply called foreach...add_entry_to_mlcl and later serialized the entire
+			 * structure. This has the disadvantage that the entire response must be in memory before
+			 * libsoup sends it to the client.
+			 *
+			 * Now, we go through the database in multiple passes (as an interim solution):
+			 *
+			 * 1. Accumulate the eventual size of the MLCL by creating and then free'ing each MLIT.
+			 * 2. Generate the DAAP preamble ending with the MLCL (with size fudged for ADBS and MLCL).
+			 * 3. Setup libsoup response headers, etc.
+			 * 4. Setup callback to transmit DAAP preamble (write_daap_preamble)
+			 * 5. Setup callback to transmit MLIT's (write_next_mlit)
+			 *    NOTE: write_next_mlit uses some tricks to iterate through all record ID's
+			 */
+
+			struct share_bitwise_t *share_bitwise;
+
+			/* 1: */
+			/* FIXME: user_data1/2 is ugly: */
+			guint32 size = 0;
+			mb.user_data1 = &size;
+			mb.user_data2 = share;
+			dmap_db_foreach (share->priv->db, (GHFunc) accumulate_mlcl_size, &mb);
+
+			/* 2: */
+			mb.mlcl = dmap_structure_add (adbs, DMAP_CC_MLCL);
+			dmap_structure_set_predicted_size (adbs, dmap_structure_get_size(adbs) + size);
+			dmap_structure_set_predicted_size (mb.mlcl, dmap_structure_get_size(mb.mlcl) + size);
+
+			/* 3: */
+			soup_message_set_status (message, SOUP_STATUS_OK);
+			soup_message_headers_set_content_length (message->response_headers, dmap_structure_get_size(adbs) + 8);
+			/* Free memory after each chunk sent out over network. */
+			soup_message_body_set_accumulate (message->response_body, FALSE);
+
+			soup_message_headers_append (message->response_headers, "Connection", "Close");
+			soup_message_headers_append (message->response_headers, "Content-Type", "application/x-daap-tagged");
+			DMAP_SHARE_GET_CLASS (share)->message_add_standard_headers (share, message);
+
+			/* 4: */
+			g_signal_connect (message, "wrote_headers", G_CALLBACK (write_daap_preamble), adbs);
+
+			/* 5: */
+			share_bitwise = g_new (struct share_bitwise_t, 1);
+			share_bitwise->share = share;
+			share_bitwise->bits = mb.bits;
+			g_signal_connect (message, "wrote_chunk", G_CALLBACK (write_next_mlit), share_bitwise);
 		}
 
-		_dmap_share_message_set_from_dmap_structure (share, message, adbs);
-		dmap_structure_destroy (adbs);
-		adbs = NULL;
 	} else if (g_ascii_strcasecmp ("/1/containers", rest_of_path) == 0) {
 	/* APLY database playlists
 	 * 	MSTT status
diff --git a/libdmapsharing/dmap-share.h b/libdmapsharing/dmap-share.h
index 947150b..b4d7f0d 100644
--- a/libdmapsharing/dmap-share.h
+++ b/libdmapsharing/dmap-share.h
@@ -112,7 +112,7 @@ typedef struct {
 	void              (*message_add_standard_headers) (DMAPShare *share,
 							   SoupMessage *msg);
 	struct DMAPMetaDataMap * (*get_meta_data_map)     (DMAPShare *share);
-	void              (*add_entry_to_mlcl)            (gpointer id,
+	guint32           (*add_entry_to_mlcl)            (gpointer id,
 							   DMAPRecord *record,
 							   gpointer mb);
 	void		  (*databases_browse_xxx)	  (DMAPShare *share,
diff --git a/libdmapsharing/dmap-structure.c b/libdmapsharing/dmap-structure.c
index aaf6fac..eeaaccd 100644
--- a/libdmapsharing/dmap-structure.c
+++ b/libdmapsharing/dmap-structure.c
@@ -785,3 +785,16 @@ dmap_structure_print (GNode *structure)
         g_node_traverse (structure, G_PRE_ORDER, G_TRAVERSE_ALL, -1, (GNodeTraverseFunc)print_dmap_item, NULL);
     }
 }
+
+guint
+dmap_structure_get_size (GNode *structure)
+{
+	return ((DMAPStructureItem *) structure->data)->size;
+}
+
+
+void
+dmap_structure_set_predicted_size (GNode *structure, guint size)
+{
+        ((DMAPStructureItem *) structure->data)->size = size;
+}
diff --git a/libdmapsharing/dmap-structure.h b/libdmapsharing/dmap-structure.h
index e0f4ebe..720a337 100644
--- a/libdmapsharing/dmap-structure.h
+++ b/libdmapsharing/dmap-structure.h
@@ -196,19 +196,21 @@ struct _DMAPStructureItem {
 	guint size;
 };
 
-GNode             *dmap_structure_add       (GNode *parent, 
-                                             DMAPContentCode cc, 
-                                             ...);
-gchar             *dmap_structure_serialize (GNode *structure, 
-                                             guint *length);
-GNode             *dmap_structure_parse     (const gchar *buf, 
-                                             gint buf_length);
-DMAPStructureItem *dmap_structure_find_item (GNode *structure, 
-                                             DMAPContentCode code);
-GNode             *dmap_structure_find_node (GNode *structure, 
-                                             DMAPContentCode code);
-void               dmap_structure_print     (GNode *structure);
-void               dmap_structure_destroy   (GNode *structure);
+GNode             *dmap_structure_add		     (GNode *parent,
+						      DMAPContentCode cc,
+						      ...);
+gchar             *dmap_structure_serialize	     (GNode *structure, 
+						      guint *length);
+GNode             *dmap_structure_parse		     (const gchar *buf, 
+						      gint buf_length);
+DMAPStructureItem *dmap_structure_find_item	     (GNode *structure, 
+						      DMAPContentCode code);
+GNode             *dmap_structure_find_node	     (GNode *structure, 
+						      DMAPContentCode code);
+void               dmap_structure_print		     (GNode *structure);
+void               dmap_structure_destroy	     (GNode *structure);
+guint		   dmap_structure_get_size	     (GNode *structure);
+void		   dmap_structure_set_predicted_size (GNode *structure, guint size);
 
 typedef enum {
 	DMAP_TYPE_BYTE = 0x0001,
diff --git a/libdmapsharing/dpap-share.c b/libdmapsharing/dpap-share.c
index 001e4f9..8b99be7 100644
--- a/libdmapsharing/dpap-share.c
+++ b/libdmapsharing/dpap-share.c
@@ -78,7 +78,7 @@ static void databases_items_xxx (DMAPShare *share,
 				 GHashTable *query,
 				 SoupClientContext *context);
 static struct DMAPMetaDataMap *get_meta_data_map (DMAPShare *share);
-static void add_entry_to_mlcl (gpointer id,
+static guint32 add_entry_to_mlcl (gpointer id,
 			       DMAPRecord *record,
 			       gpointer mb);
 
@@ -347,7 +347,7 @@ file_to_mmap (const char *location)
 	return mapped_file;
 }
 
-static void
+static guint32
 add_entry_to_mlcl (gpointer id,
 		   DMAPRecord *record,
 		   gpointer _mb)
@@ -466,7 +466,8 @@ add_entry_to_mlcl (gpointer id,
 		}
 		dmap_structure_add (mlit, DMAP_CC_PFDT, data, size);
 	}
-	return;
+
+	return ((DMAPStructureItem *) mlit->data)->size;
 }
 
 static void
diff --git a/tests/test-daap-record.c b/tests/test-daap-record.c
index b31f9be..a6bc017 100644
--- a/tests/test-daap-record.c
+++ b/tests/test-daap-record.c
@@ -233,9 +233,9 @@ test_daap_record_class_init (TestDAAPRecordClass *klass)
 
         g_object_class_override_property (gobject_class, PROP_LOCATION, "location");
         g_object_class_override_property (gobject_class, PROP_TITLE, "title");
-        g_object_class_override_property (gobject_class, PROP_ALBUM, "album");
-        g_object_class_override_property (gobject_class, PROP_ARTIST, "artist");
-        g_object_class_override_property (gobject_class, PROP_GENRE, "genre");
+        g_object_class_override_property (gobject_class, PROP_ALBUM, "songalbum");
+        g_object_class_override_property (gobject_class, PROP_ARTIST, "songartist");
+        g_object_class_override_property (gobject_class, PROP_GENRE, "songgenre");
         g_object_class_override_property (gobject_class, PROP_FORMAT, "format");
         g_object_class_override_property (gobject_class, PROP_RATING, "rating");
         g_object_class_override_property (gobject_class, PROP_FILESIZE, "filesize");
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]