Created attachment 363471 [details] [review]
avoid string allocations in g_resources_enumerate_children

Review of attachment 363471 [details] [review]:

::: gio/gresource.c
@@ +848,3 @@
                                GError               **error)
 {
+  gchar local_str[256];

Is it worth using PATH_MAX for this if defined?

@@ +866,3 @@
   if (path[path_len-1] != '/')
+    {
+      if (path_len < (sizeof local_str - 2))

Brackets for sizeof don’t match the coding style:

if (path_len < sizeof (local_str) - 2)

@@ +871,3 @@
+          local_str[path_len] = '/';
+          local_str[path_len+1] = 0;
+          path_with_slash = local_str;

I’m not so happy about this code path; it seems to be too much of a special case. If it’s going in, it needs a comment explaining why it’s a necessary fast path.

Could you change Builder to ensure that all the paths it passes in have a trailing slash? That would avoid the memcpy() as well.

Review of attachment 363471 [details] [review]:

Please also ensure that the unit tests for g_resource_enumerate_children() cover calls with and without a trailing slash, and with path lengths greater than the size of the buffer on the stack.

Review of attachment 363471 [details] [review]:

::: gio/gresource.c
@@ +848,3 @@
                                GError               **error)
 {
+  gchar local_str[256];

PATH_MAX can't be trusted and can even be max uint (not sure if we support anywhere with that). 256 was arbitrarily chosen as the size of a single cacheline on modern intel which would be "sufficient large for all paths ive used".

@@ +871,3 @@
+          local_str[path_len] = '/';
+          local_str[path_len+1] = 0;
+          path_with_slash = local_str;

Well the point of it was that g_strconcat() was showing up really high on my memory profiles as an offender of transient allocations. I would have used g_snprintf(), but what's the point in parsing a string to do a copy set two bytes.

We should still change things in our various projects to avoid it though.

Review of attachment 363471 [details] [review]:

::: gio/gresource.c
@@ +848,3 @@
                                GError               **error)
 {
+  gchar local_str[256];

OK, but please stick in a brief comment justifying why 256 was chosen (i.e. basically what you said here).

@@ +871,3 @@
+          local_str[path_len] = '/';
+          local_str[path_len+1] = 0;
+          path_with_slash = local_str;

I know what the point of it is; other people reading the code later might not. The comment need not be verbose, but should be present.

Created attachment 363512 [details] [review]
another approach

Created attachment 363513 [details] [review]
another approach, 2

maybe even more hacky, but it entirely avoids copying the string, in all cases

(In reply to Matthias Clasen from comment #8)
> maybe even more hacky, but it entirely avoids copying the string, in all
> cases

I far prefer Christian’s approach. Changing gvdb just for GResource is way too invasive. It doesn’t look like this last_byte argument could usefully be used by any other code which calls gvdb (ie. GSettings).

I’m more interested to know how feasible it is to change Builder to always include the trailing slash in its calls, and circumvent the need for all of this. (To be clear, I still think it makes sense to go with Christian’s patch, even if we do fix Builder, since this is likely a hot path for any other large GTK+ app.)

most of these calls come out of GtkIconTheme, I assume, where we share the same path for icon cache lookups and resource lookups. It should in theory be possible to make all the paths have a trailing /, but I'm quite worried that there will be fallout. GtkIconTheme is quite messy

I've taken care of all the places I could find in gtk now. Maybe that is good enough. Christian ?

Created attachment 363573 [details] [review]
gresource: avoid allocations in enumerate_children()

In the vast majority of cases, we can avoid temporary
allocations for paths in g_resources_enumerate_children().

In the case we need to add a suffix "/", we can usually just
build the path on the stack. In other cases, we can completely
avoid the strdup, which appears to only have been added for
readability. If the path is really long, we fallback to doing
what we did before, and use g_strconcat().

In the case of Builder, this saved 5.3mb of temporary
allocations in the process of showing the first application
window.

(In reply to Matthias Clasen from comment #11)
> I've taken care of all the places I could find in gtk now. Maybe that is
> good enough. Christian ?

It's probably fine. Builder and libdazzle where doing some pretty bad things because I had no idea of the need to have a trailing slash. Libpeas, which also scans resources aggressively only joins the results of this function, so it should get the trailing slash automatically.

I don't care which approach we take since I don't have to maintain it, but I've uploaded another patch with the nits addressed.

Review of attachment 363573 [details] [review]:

(In reply to Philip Withnall from comment #3)
> Review of attachment 363471 [details] [review] [review]:
> 
> Please also ensure that the unit tests for g_resource_enumerate_children()
> cover calls with and without a trailing slash, and with path lengths greater
> than the size of the buffer on the stack.

Still need some tests, please. (Sorry, my original comment was not very obvious.)

The rest looks good to me.

Created attachment 363645 [details] [review]
gresource: avoid allocations in enumerate_children()

In the vast majority of cases, we can avoid temporary
allocations for paths in g_resources_enumerate_children().

In the case we need to add a suffix "/", we can usually just
build the path on the stack. In other cases, we can completely
avoid the strdup, which appears to only have been added for
readability. If the path is really long, we fallback to doing
what we did before, and use g_strconcat().

In the case of Builder, this saved 5.3mb of temporary
allocations in the process of showing the first application
window.

Review of attachment 363645 [details] [review]:

Great, thanks.

Attachment 363645 [details] pushed as 5464461 - gresource: avoid allocations in enumerate_children()