GNOME.org
 

Re: Reusing message info instances after remapping

This one seems to work.

Can people please test this (a LOT)?

Just apply it, go online (for example on the test account), and remove
messages. Try to make it crash and/or try to make it NOT show a message,
to much messages, duplicate messages or do wieerd things that shouldn't
happen.

And even better, figure out why it happens.

Also measure memory usage (it's going to be a little bit higher).


On Thu, 2006-11-09 at 14:36 +0100, Philip Van Hoof wrote:
> This is a little bit a difficult to explain subject. Rather technical.
> 
> What will happen on camel_folder_summary_save is that the mmap will
> reload itself.
> 
> This reloading currently effectively makes all the message-info
> instances being used by the TnyCamelHeader instances incorrect and
> hazardous.
> 
> That's why it's not possible to expunge a folder while still having
> headers being managed. I added an assertion check for that. It will
> simply deny the expunge request.
> 
> That's because expunging means saving the summary. And saving the
> summary means reloading the mmap.
> 
> This is a rather icky limitation for the developer's point of view: he
> has to close the folder completely (destroy all header instances) before
> he can expunge it.
> 
> 
> This patch is a first attempt into allowing expunges to happen on open
> folders. Headers of removed messages will simply end up pointing to a
> NULL pointer for their message-info instance. And existing message-info
> instances will be reused by the code that parses trough the mmapped data
> and assigns the pointers which are defined in the structure.
> 
> It currently does this by searching for an original one that had the
> same uid, and then in stead of destroying the instance, reusing it
> (correcting it with the new mmap offsets after the file is mmapped).
> 
> There are a few glitches in it (it does work, but some headers aren't
> found and things like that).
> 
> I would love others to take a look at this. It's very tricky things that
> I have to do. I will probably get it 100% perfect during the weekend or
> something. But feel free to send me improved versions of this patch.
> 
> I do warn that it's icky tricky code. A bit hard to get it right. You
> need to fully understand how I did the mmap trick. Stuff like that.
> 
> Nevertheless I'm very interested in the opinions of you guys.
> 
> 
> ps. YES, I know I should use a factory. And feel free to put that in the
> patch if you believe that will give you more control over the decision
> of recreation or reuse of an instance. Do note that each allocation here
> is extremely expensive: you need to repeat that allocation FOR EVERY
> header (so 50,000 headers means 50,000 such allocations. Which becomes
> quickly extremely expensive). (and I know that a factory doesn't imply
> these extra allocations, just keep it in the back of your head).
> 
> ps. There's a hashtable called messages_uid which might be useful for
> replacing the search code. On the other hand I'm trying to remove that
> hashtable too (it's needlessly consuming memory for a feature that
> tinymail does not really use).
> 
> 
> Go for it :). Don't forget to have fun
> 
> 
> _______________________________________________
> tinymail-devel-list mailing list
> tinymail-devel-list gnome org
> http://mail.gnome.org/mailman/listinfo/tinymail-devel-list
-- 
Philip Van Hoof, software developer
home: me at pvanhoof dot be
gnome: pvanhoof at gnome dot org
work: vanhoof at x-tend dot be
blog: http://pvanhoof.be/blog

Index: tny-camel-folder.c
===================================================================
--- tny-camel-folder.c	(revision 1112)
+++ tny-camel-folder.c	(working copy)
@@ -273,7 +273,7 @@
 	TnyCamelFolderPriv *priv = TNY_CAMEL_FOLDER_GET_PRIVATE (self);
 	CamelException ex = CAMEL_EXCEPTION_INITIALISER;
 
-	if (priv->headers_managed > 0)
+	if (FALSE && priv->headers_managed > 0)
 	{
 		g_critical ("Request to expunge a folder denied: there are still "
 			"header instances of this folder active. Destroy them "
Index: camel-lite/camel/providers/imap/camel-imap-summary.c
===================================================================
--- camel-lite/camel/providers/imap/camel-imap-summary.c	(revision 1112)
+++ camel-lite/camel/providers/imap/camel-imap-summary.c	(working copy)
@@ -40,7 +40,7 @@
 static int summary_header_load (CamelFolderSummary *);
 static int summary_header_save (CamelFolderSummary *, FILE *);
 
-static CamelMessageInfo *message_info_load (CamelFolderSummary *s);
+static CamelMessageInfo *message_info_load (CamelFolderSummary *s, gboolean *must_add);
 static int message_info_save (CamelFolderSummary *s, FILE *out,
 			      CamelMessageInfo *info);
 static gboolean info_set_user_tag(CamelMessageInfo *info, const char *name, const char  *value);
@@ -206,12 +206,12 @@
 }
 
 static CamelMessageInfo *
-message_info_load (CamelFolderSummary *s)
+message_info_load (CamelFolderSummary *s, gboolean *must_add)
 {
 	CamelMessageInfo *info;
 	CamelImapMessageInfo *iinfo;
 
-	info = camel_imap_summary_parent->message_info_load (s);
+	info = camel_imap_summary_parent->message_info_load (s, must_add);
 	iinfo = (CamelImapMessageInfo*)info;
 
 	if (info) {
Index: camel-lite/camel/providers/imapp/camel-imapp-summary.c
===================================================================
--- camel-lite/camel/providers/imapp/camel-imapp-summary.c	(revision 1112)
+++ camel-lite/camel/providers/imapp/camel-imapp-summary.c	(working copy)
@@ -40,7 +40,7 @@
 static int summary_header_load(CamelFolderSummary *);
 static int summary_header_save(CamelFolderSummary *, FILE *);
 
-static CamelMessageInfo *message_info_load(CamelFolderSummary *s);
+static CamelMessageInfo *message_info_load(CamelFolderSummary *s, gboolean *must_add);
 static int message_info_save(CamelFolderSummary *s, FILE *out, CamelMessageInfo *info);
 
 static void camel_imapp_summary_class_init(CamelIMAPPSummaryClass *klass);
@@ -159,12 +159,12 @@
 
 
 static CamelMessageInfo *
-message_info_load(CamelFolderSummary *s)
+message_info_load(CamelFolderSummary *s, gboolean *must_add)
 {
 	CamelMessageInfo *info;
 	CamelIMAPPMessageInfo *iinfo;
 
-	info = camel_imapp_summary_parent->message_info_load(s);
+	info = camel_imapp_summary_parent->message_info_load(s, must_add);
 	if (info) {
 		unsigned char *ptrchr = s->filepos;
 		iinfo =(CamelIMAPPMessageInfo *)info;
Index: camel-lite/camel/providers/local/camel-mbox-summary.c
===================================================================
--- camel-lite/camel/providers/local/camel-mbox-summary.c	(revision 1112)
+++ camel-lite/camel/providers/local/camel-mbox-summary.c	(working copy)
@@ -57,7 +57,7 @@
 
 static CamelMessageInfo * message_info_new_from_header(CamelFolderSummary *, struct _camel_header_raw *);
 static CamelMessageInfo * message_info_new_from_parser(CamelFolderSummary *, CamelMimeParser *);
-static CamelMessageInfo * message_info_load (CamelFolderSummary *);
+static CamelMessageInfo * message_info_load (CamelFolderSummary *, gboolean *);
 static int		  message_info_save (CamelFolderSummary *, FILE *, CamelMessageInfo *);
 static int 		  meta_message_info_save(CamelFolderSummary *s, FILE *out_meta, FILE *out, CamelMessageInfo *mi);
 /*static void		  message_info_free (CamelFolderSummary *, CamelMessageInfo *);*/
@@ -378,13 +378,13 @@
 
 
 static CamelMessageInfo *
-message_info_load(CamelFolderSummary *s)
+message_info_load(CamelFolderSummary *s, gboolean *must_add)
 {
 	CamelMessageInfo *mi;
 
 	io(printf("loading mbox message info\n"));
 
-	mi = ((CamelFolderSummaryClass *)camel_mbox_summary_parent)->message_info_load(s);
+	mi = ((CamelFolderSummaryClass *)camel_mbox_summary_parent)->message_info_load(s, must_add);
 	if (mi) {
 		CamelMboxMessageInfo *mbi = (CamelMboxMessageInfo *)mi;
 
Index: camel-lite/camel/providers/local/camel-maildir-summary.c
===================================================================
--- camel-lite/camel/providers/local/camel-maildir-summary.c	(revision 1112)
+++ camel-lite/camel/providers/local/camel-maildir-summary.c	(working copy)
@@ -48,7 +48,7 @@
 
 #define CAMEL_MAILDIR_SUMMARY_VERSION (0x2000)
 
-static CamelMessageInfo *message_info_load(CamelFolderSummary *s);
+static CamelMessageInfo *message_info_load(CamelFolderSummary *s, gboolean *must_add);
 static CamelMessageInfo *message_info_new_from_header(CamelFolderSummary *, struct _camel_header_raw *);
 static void message_info_free(CamelFolderSummary *, CamelMessageInfo *mi);
 
@@ -385,12 +385,12 @@
 }
 
 static CamelMessageInfo *
-message_info_load(CamelFolderSummary *s)
+message_info_load(CamelFolderSummary *s, gboolean *must_add)
 {
 	CamelMessageInfo *mi;
 	CamelMaildirSummary *mds = (CamelMaildirSummary *)s;
 
-	mi = ((CamelFolderSummaryClass *) parent_class)->message_info_load(s);
+	mi = ((CamelFolderSummaryClass *) parent_class)->message_info_load(s, must_add);
 	if (mi) {
 		char *name;
 
Index: camel-lite/camel/camel-folder-summary.c
===================================================================
--- camel-lite/camel/camel-folder-summary.c	(revision 1112)
+++ camel-lite/camel/camel-folder-summary.c	(working copy)
@@ -99,7 +99,7 @@
 static CamelMessageInfo * message_info_new_from_header(CamelFolderSummary *, struct _camel_header_raw *);
 static CamelMessageInfo * message_info_new_from_parser(CamelFolderSummary *, CamelMimeParser *);
 static CamelMessageInfo * message_info_new_from_message(CamelFolderSummary *s, CamelMimeMessage *msg);
-static CamelMessageInfo * message_info_load(CamelFolderSummary *);
+static CamelMessageInfo * message_info_load(CamelFolderSummary *, gboolean *must_add);
 static int		  message_info_save(CamelFolderSummary *, FILE *, CamelMessageInfo *);
 static void		  message_info_free(CamelFolderSummary *, CamelMessageInfo *);
 
@@ -125,6 +125,12 @@
 
 static CamelObjectClass *camel_folder_summary_parent;
 
+static void 
+messages_uid_destroy_key (gpointer key)
+{
+	g_free (key);
+}
+
 static void
 camel_folder_summary_init (CamelFolderSummary *s)
 {
@@ -145,10 +151,10 @@
 	s->flags = 0;
 	s->time = 0;
 	s->nextuid = 1;
+	s->in_reload = FALSE;
 
-
 	s->messages = g_ptr_array_new();
-	s->messages_uid = g_hash_table_new(g_str_hash, g_str_equal);
+	s->messages_uid = g_hash_table_new_full (g_str_hash, g_str_equal, messages_uid_destroy_key, NULL);
 	
 	p->summary_lock = g_mutex_new();
 	p->io_lock = g_mutex_new();
@@ -181,17 +187,21 @@
 
 	p = _PRIVATE(s);
 
-	camel_folder_summary_clear(s);
+	/* camel_folder_summary_clear(s); */
+
 	if (s->file)
 		g_mapped_file_free (s->file);
 	s->file = NULL;
+/*
+	camel_folder_summary_remove_range (s, 0, s->messages->len-1);
+	// g_ptr_array_foreach (s->messages, foreach_msginfo, (gpointer)s->message_info_size);
+	//if (s->messages->len > 0)
+	//	g_ptr_array_remove_range (s->messages, 0, s->messages->len-1);
+	//g_hash_table_foreach_remove (s->messages_uid, always_true, NULL);
+*/
 
-	g_ptr_array_foreach (s->messages, foreach_msginfo, (gpointer)s->message_info_size);
-	if (s->messages->len > 0)
-		g_ptr_array_remove_range (s->messages, 0, s->messages->len-1);
-	g_hash_table_foreach_remove (s->messages_uid, always_true, NULL);
 	g_hash_table_foreach(p->filter_charset, free_o_name, 0);
-	
+
 	if (p->filter_index)
 		camel_object_unref((CamelObject *)p->filter_index);
 	if (p->filter_64)
@@ -209,6 +219,7 @@
 		camel_object_unref((CamelObject *)p->filter_stream);
 	if (p->index)
 		camel_object_unref((CamelObject *)p->index);
+
 }
 
 static void
@@ -610,7 +621,6 @@
 	int i;
 	CamelMessageInfo *mi;
 
-
 	if (s->summary_path == NULL || !g_file_test (s->summary_path, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_REGULAR))
 		return 0;
 
@@ -626,7 +636,8 @@
 
 	/* now read in each message ... */
 	for (i=0;i<s->saved_count;i++) {
-		mi = ((CamelFolderSummaryClass *)(CAMEL_OBJECT_GET_CLASS(s)))->message_info_load(s);
+		gboolean must_add = FALSE;
+		mi = ((CamelFolderSummaryClass *)(CAMEL_OBJECT_GET_CLASS(s)))->message_info_load(s, &must_add);
 
 		if (mi == NULL)
 			goto error;
@@ -641,7 +652,8 @@
 			}
 		}
 
-		camel_folder_summary_mmap_add(s, mi);
+		if (must_add)
+			camel_folder_summary_mmap_add(s, mi);
 	}
 
 
@@ -741,13 +753,20 @@
 
 	for (i = 0; i < count; i++) {
 		mi = s->messages->pdata[i];
+
 		if (((CamelFolderSummaryClass *)(CAMEL_OBJECT_GET_CLASS (s)))->message_info_save (s, out, mi) == -1)
 			goto exception;
-		
+
 		if (s->build_content) {
 			if (perform_content_info_save (s, out, ((CamelMessageInfoBase *)mi)->content) == -1)
 				goto exception;
 		}
+
+		if (! (((CamelMessageInfoBase*)mi)->flags & CAMEL_MESSAGE_INFO_UID_NEEDS_FREE))
+		{
+			mi->uid = g_strdup (mi->uid);
+			((CamelMessageInfoBase*)mi)->flags |= CAMEL_MESSAGE_INFO_UID_NEEDS_FREE;
+		}
 	}
 
 	if (fflush (out) != 0 || fsync (fileno (out)) == -1)
@@ -757,6 +776,8 @@
 	
 	fclose (out);
 
+	s->in_reload = TRUE;
+
 	camel_folder_summary_unload_mmap (s);
 #ifdef G_OS_WIN32
 	g_unlink(s->summary_path);
@@ -769,6 +790,8 @@
 	}
 	camel_folder_summary_load (s);
 
+	s->in_reload = FALSE;
+
 	s->flags &= ~CAMEL_SUMMARY_DIRTY;
 
 	g_mutex_unlock (s->dump_lock);
@@ -881,6 +904,8 @@
 void
 camel_folder_summary_add(CamelFolderSummary *s, CamelMessageInfo *info)
 {
+	gchar *uidn;
+
 	g_mutex_lock (s->dump_lock);
 
 	if (info == NULL) 
@@ -905,7 +930,8 @@
 #endif
 
 	g_ptr_array_add(s->messages, info);
-	g_hash_table_insert(s->messages_uid, (char *)camel_message_info_uid(info), info);
+	uidn = g_strdup (camel_message_info_uid(info));
+	g_hash_table_insert(s->messages_uid, uidn, info);
 	s->flags |= CAMEL_SUMMARY_DIRTY;
 
 	CAMEL_SUMMARY_UNLOCK(s, summary_lock);
@@ -916,6 +942,8 @@
 static void
 camel_folder_summary_mmap_add(CamelFolderSummary *s, CamelMessageInfo *info)
 {
+	gchar *uidn;
+
 	CAMEL_SUMMARY_LOCK(s, summary_lock);
 
 /* unnecessary for pooled vectors */
@@ -926,7 +954,8 @@
 #endif
 
 	g_ptr_array_add(s->messages, info);
-	g_hash_table_insert(s->messages_uid, (char *)camel_message_info_uid(info), info);
+	uidn = g_strdup (camel_message_info_uid(info));
+	g_hash_table_insert(s->messages_uid, uidn, info);
 	s->flags |= CAMEL_SUMMARY_DIRTY;
 
 	CAMEL_SUMMARY_UNLOCK(s, summary_lock);
@@ -1869,26 +1898,59 @@
 
 
 static CamelMessageInfo *
-message_info_load(CamelFolderSummary *s)
+message_info_load(CamelFolderSummary *s, gboolean *must_add)
 {
-	CamelMessageInfoBase *mi;
+	CamelMessageInfoBase *mi = NULL;
 	guint count, len;
 	unsigned char *ptrchr = s->filepos;
 	unsigned int i;
+	gchar *theuid;
 
 #ifndef NON_TINYMAIL_FEATURES
 	unsigned int size;
 #endif
 
-	mi = (CamelMessageInfoBase *)camel_message_info_new(s);
-
 	io(printf("Loading message info\n"));
 
 	ptrchr = camel_file_util_mmap_decode_uint32 (ptrchr, &len, TRUE);
 	if (len) 
-		mi->uid = (char*)ptrchr;
+		theuid = (char*)ptrchr;
 	ptrchr += len;
 
+	if (!s->in_reload)
+	{
+		mi = (CamelMessageInfoBase *)camel_message_info_new(s);
+		*must_add = TRUE;
+		mi->uid = theuid;
+	} else
+	{
+		CamelMessageInfoBase *ni = (CamelMessageInfoBase*) camel_folder_summary_uid (s, theuid);
+		
+		if (ni)
+		{
+			printf ("Found %s\n", theuid);
+			//if (ni->flags & CAMEL_MESSAGE_INFO_UID_NEEDS_FREE)
+			//{
+			//	g_free (ni->uid);
+			//	ni->flags &= ~CAMEL_MESSAGE_INFO_UID_NEEDS_FREE;
+			//}
+			mi = ni;
+		}
+	}
+
+	i = 0;
+
+	if (!mi)
+	{
+		printf ("%s not found\n", theuid);
+		mi = (CamelMessageInfoBase *)camel_message_info_new(s);
+		*must_add = TRUE;
+		
+		mi->uid = theuid;
+	}
+
+	
+
 	ptrchr = camel_file_util_mmap_decode_uint32 (ptrchr, &mi->flags, FALSE);
 
 	mi->flags &= ~CAMEL_MESSAGE_INFO_UID_NEEDS_FREE;
Index: camel-lite/camel/camel-folder-summary.h
===================================================================
--- camel-lite/camel/camel-folder-summary.h	(revision 1112)
+++ camel-lite/camel/camel-folder-summary.h	(working copy)
@@ -235,7 +235,7 @@
 	char *summary_path;
 	gboolean build_content;	/* do we try and parse/index the content, or not? */
 
-	GPtrArray *messages;	/* CamelMessageInfo's */
+	GPtrArray *messages; /* CamelMessageInfo's */
 	GHashTable *messages_uid; /* CamelMessageInfo's by uid */
 
 	struct _CamelFolder *folder; /* parent folder, for events */
@@ -244,6 +244,7 @@
 	GMappedFile *file;
 	unsigned char *filepos;
 	GMutex *dump_lock;
+	gboolean in_reload;
 };
 
 struct _CamelFolderSummaryClass {
@@ -257,7 +258,7 @@
 	CamelMessageInfo * (*message_info_new_from_header)(CamelFolderSummary *, struct _camel_header_raw *);
 	CamelMessageInfo * (*message_info_new_from_parser)(CamelFolderSummary *, CamelMimeParser *);
 	CamelMessageInfo * (*message_info_new_from_message)(CamelFolderSummary *, CamelMimeMessage *);
-	CamelMessageInfo * (*message_info_load)(CamelFolderSummary *);
+	CamelMessageInfo * (*message_info_load)(CamelFolderSummary *, gboolean *must_add);
  	int		   (*message_info_save)(CamelFolderSummary *, FILE *, CamelMessageInfo *);
 	int		   (*meta_message_info_save)(CamelFolderSummary *, FILE *, FILE *, CamelMessageInfo *);
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]