Re: [Evolution-hackers] mmap patch
I'm just going to play open cards in my reply. No more contests, no more "look how good I am". Just my true intentions, on a plate. I promised Harish I was going to change that attitude. So I will. I will also technically comment where necessary. On Wed, 2006-07-19 at 16:29 -0400, Jeffrey Stedfast wrote: > Well, I figured out why you're getting alignment problems... you're > using int* pointers to decode stuff. You need to use a char* - technical - There's no data alignment problems in the latest patch. http://pvanhoof.be/files/evolution_data_server__mmap_summary.diff > Secondly, your code is just really gross: > > - Why do you have a filepos pointer? it's completely unnecessary. - technical - Because it's a hack. Initially I didn't want to change the internal API, so I had no other options but to put it in the CamelFolderSummary struct. After federico's three points I , however, changed the internal API by removing the FILE* from the methods that aren't using it anymore. A better way would have been to pass the mmap addr position here, in stead of the FILE*. Note that the current patch isn't intended for HEAD. It is (for me) mainly intended for testing purposes. If upstream is really interested, they will probably change the summary file-format drastically anyway. You where the first to say that for mmap it would be better to change that file format. I was the first to acknowledge that. I guess we are in agreement here. - open cards - In fact, Jeff, if you want to know why I really really started the work: I did it because I wanted to get things moving in Camel-land. Because people, like maybe you, would be awaken and/or become passionate about wanting to do the disk-summary branch so badly ... that they will just start making it really happen. It might surprise you, but in a way I was also happy that you said you questioned this patch by mentioning the disk-summary branch. I hope I inspired you to go for it. In another way I was of course a little bit pissed. But it's passion that moves things. Anger that makes passion. You fueled me to implement the three points federico asked for. Without your fuel, I wouldn't have done that. I really believe in the concepts you and NotZed outlined with the disk summary idea. Even a little bit more than I believe in my own mmap ideas. The combination however ... a combination would be awesome. I really think that a "just go for it"-team can make this happen. I really believe that you, Jeffrey, could steer such a team. I might often present myself as an asshole. My intention is to get things moving. I want us to make things better. A lot better. Just because we can. For me, that's definitely a reason why we should. Evolution doesn't have to bleed to death (which is btw what a lot people predict to happen). There's indeed a lot good stuff in it. Good stuff that you guys made happen. I acknowledge that, if that is what you wanted to hear from me. I too can read code myself and figure out who the authors are. I too respect their work. I also respect the new guys. > - Why do you use a ptr32 thing? That's gross. There are far cleaner ways > of doing this... it's also the source of your alignment problems. - technical - All the "ptr32" variables are removed in the latest patch, they've been replaced with the get_unaligned_u32 macro. Are you sure you checked the latest patch? If I grep for "ptr32" in the latest patch, there's no occurrences. > - Why do you have ::needs_free and ::uid_needs_free? Please find another > way to do this. This is a horrible hack. > > Attached you'll find the beginnings of a much cleaner implementation of > the mmap approach. - open cards - That's really great, you cleaning this up means you are considering the patch. That is my intention. It's good to know that you are at least looking at it. - technical - What matters for me (and also for tinymail) is that the data isn't allocated by malloc if it doesn't need to be allocated. If it can be read from by mmap, it's better for mobile devices to read it that way. >From the beginning I was very uncertain that the patch would be fit for daily Evolution usage. I turned out to just work. A reason for the ugly pieces of that patch, is that uncertainty. - open cards - Initially this was, indeed, a tryout. An experiment. A hack. I openly admit that I had no idea if it would work with Evolution. I was concerned people would look at me as somebody who's only intention was to fork Camel for tinymail. Therefore I added four days of hacking to the patch to make it work on Evolution. I didn't have to do that, because after the second day .. it worked with tinymail already. You can follow the dates in my blog. If I wanted to be that forking asshole, I would have done a camel-lite (oh wait, I did). Anyway .. the point that I'm trying to make is: I wouldn't have tried to make it work with Evolution. Nobody would have cared a lot, and I wouldn't have to write this open-card
[Evolution-hackers] mmap patch
Well, I figured out why you're getting alignment problems... you're using int* pointers to decode stuff. You need to use a char* Secondly, your code is just really gross: - Why do you have a filepos pointer? it's completely unnecessary. - Why do you use a ptr32 thing? That's gross. There are far cleaner ways of doing this... it's also the source of your alignment problems. - Why do you have ::needs_free and ::uid_needs_free? Please find another way to do this. This is a horrible hack. Attached you'll find the beginnings of a much cleaner implementation of the mmap approach. -- Jeffrey Stedfast Evolution Hacker - Novell, Inc. [EMAIL PROTECTED] - www.novell.com ? camel-gpg-context.c.gpg ? camel-mime-tables.c ? camel_folder_summary_with_mmap_fixes11.txt ? const-charset-map.patch ? mmap.patch ? pgp-filter.patch ? pstring.patch ? providers/imap4/272058.patch ? providers/imap4/imap4-sockopt.patch ? providers/local/largefile-mbox.patch ? providers/pop3/pop3-folder.c Index: camel-file-utils.c === RCS file: /cvs/gnome/evolution-data-server/camel/camel-file-utils.c,v retrieving revision 1.17 diff -u -r1.17 camel-file-utils.c --- camel-file-utils.c 2 Jun 2006 00:52:29 - 1.17 +++ camel-file-utils.c 19 Jul 2006 20:32:13 - @@ -262,18 +262,18 @@ int camel_file_util_encode_string (FILE *out, const char *str) { - register int len; - - if (str == NULL) - return camel_file_util_encode_uint32 (out, 1); + size_t len; - if ((len = strlen (str)) > 65536) - len = 65536; + if (str == NULL) + return camel_file_util_encode_size_t (out, 0); - if (camel_file_util_encode_uint32 (out, len+1) == -1) + len = strlen (str); + if (camel_file_util_encode_size_t (out, len + 1) == -1) return -1; - if (len == 0 || fwrite (str, len, 1, out) == 1) + + if (fwrite (str, len + 1, 1, out) == 1) return 0; + return -1; } @@ -290,29 +290,317 @@ int camel_file_util_decode_string (FILE *in, char **str) { - guint32 len; - register char *ret; - - if (camel_file_util_decode_uint32 (in, &len) == -1) { + size_t len; + + if (camel_file_util_decode_size_t (in, &len) == -1) { *str = NULL; return -1; } - - len--; - if (len > 65536) { + + if (len == 0) { + *str = NULL; + return 0; + } + + if (!(*str = g_try_malloc (len))) { *str = NULL; return -1; } - - ret = g_malloc (len+1); - if (len > 0 && fread (ret, len, 1, in) != 1) { - g_free (ret); + + if (fread (*str, len, 1, in) != 1) { + g_free (*str); *str = NULL; return -1; } + + return 0; +} + + +/** + * camel_mmap_util_encode_uint32: + * @out: file to output to + * @value: value to output + * + * Utility function to save an uint32 to a file. + * + * Return value: 0 on success, -1 on error. + **/ +int +camel_mmap_util_encode_uint32 (char **out, guint32 value) +{ + char *outptr = *out; + unsigned char c; + int i; + + for (i = 28; i > 0; i -= 7) { + if (value >= (1 << i)) { + c = (value >> i) & 0x7f; + *outptr++ = c; + } + } + + c = value & 0x7f; + *outptr++ = c | 0x80; + + *out = outptr; + + return 0; +} + + +/** + * camel_mmap_util_decode_uint32: + * @in: file to read from + * @dest: pointer to a variable to store the value in + * + * Retrieve an encoded uint32 from a file. + * + * Return value: 0 on success, -1 on error. @*dest will contain the + * decoded value. + **/ +int +camel_mmap_util_decode_uint32 (const char **in, guint32 *dest) +{ + const char *inptr = *in; +guint32 value = 0; + unsigned char v; + +/* until we get the last byte, keep decoding 7 bits at a time */ + while (((v = *inptr++) & 0x80) == 0) { + value |= v; + value <<= 7; + } + + *dest = value | (v & 0x7f); + *in = inptr; + +return 0; +} + + +/** + * camel_mmap_util_encode_fixed_int32: + * @out: file to output to + * @value: value to output + * + * Encode a gint32, performing no compression, but converting + * to network order. + * + * Return value: 0 on success, -1 on error. + **/ +int +camel_mmap_util_encode_fixed_int32 (char **out, gint32 value) +{ + gint32 save; + + save = GINT32_TO_LE (value); + memcpy (*out, &save, sizeof (gint32)); + *out = *out + sizeof (gint32); + + return 0; +} + + +/** + * camel_mmap_util_decode_fixed_int32: + * @in: file to read from + * @dest: pointer to a variable to store the value in + * + * Retrieve a gint32. + * + * Return value: 0 on success, -1 on error. + **/ +int +camel_mmap_util_decode_fixed_int32 (const char **in, gint32 *dest) +{ + const char *inptr = *in; + gint32 value = 0; + int i; + + for (i = 0; i < sizeof (guint32); i++, inptr++, value <<= 8) + value |= *inptr; + + *dest = GUINT32_FROM_LE (value); + *in = inptr; + + return 0; +} + +#define MMAP_ENCODE_T(type) \ +int \ +camel_mmap_util_encode_##type(char **out, type value) \ +{ \ + type save; \ + \ + switch (sizeof (type)) { \ + case 4:\ + save = GUINT32_TO_LE (value);\ + break; \ + case 8: