Created attachment 107817 [details] [review]
use 1024 bytes of initial buffer

Created attachment 107820 [details] [review]
[PATCH] More string fixes

this is a new version of the patch above, with different buffer sizes; it also removes some of the usage of g_string_append_printf() in favour of g_strconcat().

the buffer sizes are:
- 1024 for the metadata section
- 4096 for the bookmark item
- 4096 for the whole bookmark file - maybe it should be upped to 8192
- 512 for the variable expansion when getting the application exec line

if the profiles go down, a more thorough pass at using g_strconcat() is definitely in order.

also, the spec should be amended to use the ISO8601 notation for the timestamp attribute in the application element, to avoid the only real place where we need a strdup_printf().

Wrote a small testcase that simply reads my 5.2M ~/.recently-used.xbel file and write it out to a different file. Sysprof'ed trunk v/s your patch, and this is what I got:

Function                 Before     After
g_bookmark_file_dump     20.37      16.75
g_string_append_printf    8.41       3.14
g_string_maybe_expand     5.23       2.97
g_string_sized_new        4.49       4.01
g_strconcat                  -       1.22

So, it seems there is a good gain with this.

Also, note that there are other places where constant strings could be "pseudo"-concatenated without the need of g_strconcat, like

+  buffer = g_strconcat ("  <",
+                        XBEL_BOOKMARK_ELEMENT,
+                        " ",
+                        XBEL_HREF_ATTRIBUTE, "=\"", escaped_uri, "\" ",
+                        XBEL_ADDED_ATTRIBUTE, "=\"", added, "\" ",
+                        XBEL_MODIFIED_ATTRIBUTE, "=\"", modified, "\" ",
+                        XBEL_VISITED_ATTRIBUTE, "=\"", visited, "\">\n",
+                        NULL);


could be replaced with

+  buffer = g_strconcat ("  <",
+                        XBEL_BOOKMARK_ELEMENT,
+                        " "
+                        XBEL_HREF_ATTRIBUTE "=\"", escaped_uri, "\" "
+                        XBEL_ADDED_ATTRIBUTE "=\"", added, "\" ",
+                        XBEL_MODIFIED_ATTRIBUTE "=\"", modified, "\" "
+                        XBEL_VISITED_ATTRIBUTE "=\"", visited, "\">\n",
+                        NULL);

Created attachment 107839 [details] [review]
more string fixes

This patch removes all the g_string_append_printf calls. Above table updated:

Function                 Before   After 1   Current
g_bookmark_file_dump     20.37      16.75     13.68
g_string_append_printf    8.41       3.14         -
g_string_maybe_expand     5.23       2.97      4.90
g_string_sized_new        4.49       4.01      7.09
g_strconcat                  -       1.22      1.01

Eitherway, I'd take this numbers with a grain of salt (there's no way for g_strconcat time to decrease between the first and second patches), but for reference, things seem to improve. If you have any suggestion on how to measure this more reliably, I'd love to hear.

By the way, this also fixes some small glitches in your patch regarding the indentation of some closer tags of the output xml file.

the patch looks good to me in any case as a general cleanup.

g_strconcat() with your second patch has a lot less strings to concatenate, so that might account for the decreased time spent looping in the varargs. a way to get more accurate numbers would probably be dumping a small file (~1 meg) multiple times (~100), but I think that just replacing g_string_append_printf() with g_strconcat() is a win.

please, commit to trunk.

Thanks. Committed to trunk (rev. 6752):

2008-03-22  Claudio Saavedra  <csaavedra@gnome.org>

	Bug 523877 – gbookmarkfile: avoid using g_string_append_printf() and
	other optimizations

	* glib/gbookmarkfile.c: (bookmark_metadata_dump),
	(bookmark_item_dump), (g_bookmark_file_dump), (expand_exec_line):
	Replace all calls to g_string_append_printf with g_strconcat () or
	g_string_append () where appropriate, to reduce the file creation time.
	Also, use g_string_sized_new () with an appropriate buffer size instead
	of g_string_new (NULL), to reduce time spent in memory reallocation.
	(#523877, Claudio Saavedra, Emmanuele Bassi)

But seriously, if GBookmarkFile becomes a performance problem in your application, then perhaps something is wrong in your application. In an application like eog, for example, where saving the bookmark file slows down image switching, you should perhaps consider to keep a queue of entries that need to be added to the bookmark file. When switching the image, just append to that queue. Then, when the application becomes idle, you could write out all pending changes to the bookmark file in a single operation.

Sorry, I just realized that GtkRecentManager doesn't offer an API that would allow to do bulk changes. Perhaps all (delaying and combining additions and removals) should rather be done inside GtkRecentManager.

(In reply to comment #8)
> But seriously, if GBookmarkFile becomes a performance problem in your
> application, then perhaps something is wrong in your application. In an
> application like eog, for example, where saving the bookmark file slows down
> image switching, you should perhaps consider to keep a queue of entries that
> need to be added to the bookmark file.

We do that to some extent. We update the recent files menu (and with that all the involved infrastructure) in an idle call. The problem is that still affects browsing, because it delays the load of the next image, if you switch *before* the bookmark file is updated with the current picture.

(In reply to comment #9)
> Sorry, I just realized that GtkRecentManager doesn't offer an API that would
> allow to do bulk changes. Perhaps all (delaying and combining additions and
> removals) should rather be done inside GtkRecentManager.

That would be interesting, indeed. I recognize that this something that can be improved in different layers. Making GBookmarkFile really fast is not enough.   Improving the GtkRecentManager API would be the next step, I guess.

since this bug is closed, I've opened bug 524219 for adding bulk addition support to GtkRecentManager.