GNOME Bugzilla – Bug 790310
speedup path canonicalization in GResourceFile
Last modified: 2017-11-14 23:14:32 UTC
Created attachment 363554 [details] [review] speedup path canonicalization for gresourcefile The previous version did a lot of strlen() and memmove(), which can be avoided. I happened across the code and decided to spend a few minutes cleaning it up while trying to grok what it was doing. Not essential, but it's faster and easier to read what is going on.
Looks good to me.
Review of attachment 363554 [details] [review]: > functions. memrchr() was not used due to it's lack of portability. its
Review of attachment 363554 [details] [review]: Thanks for this, it looks good to me, but I’d like to see some unit tests (and a fix for Bastien’s nitpick) before it gets merged. ::: gio/gresourcefile.c @@ +163,3 @@ + +static char * +canonicalize_filename (const char *in) It would be good to have a documentation comment summarising what ‘canonicalisation’ entails. I’d also like some unit tests which call g_resource_file_new_for_path() with all kinds of non-canonical filename to get good coverage of the new canonicalize_filename().
Created attachment 363577 [details] [review] gresourcefile: simplify path canonicalization Previously, the path canonicalization for resources had liberal use of strlen() and memmove() while walking through the path. This patch avoids any secondary strlen() and removes all use of memmove(). A single allocation is created up front as we should only ever need one additional byte more than then length of the incoming path string. To keep the implementation readable, the mechanics are kept in external functions. memrchr() was not used due to its lack of portability. This is faster in every test case I've tested. Paths that contain relative ../ have the most speedup.
The updated patch includes documentation on the canonical form and adds a new unit test for a number of degenerate cases. Commit message also fixed.
Review of attachment 363577 [details] [review]: That’s great, thanks. One minor comment, but you can fix that before pushing. ::: gio/tests/resources.c @@ +159,3 @@ +test_resource_file_path (void) +{ + static struct { `const`
Attachment 363577 [details] pushed as 38ffcd2 - gresourcefile: simplify path canonicalization