threads / patch / 46329

patchsha1_file: add slash once in for_each_file_in_obj_subdir()

Subject: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()

## tl;dr

7 messages between Jul 8, 2017 and Jul 12, 2017. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

René Scharfe· Jul 8, 2017, 08:59 UTC · lore

Add the slash between loose object subdirectory and file name just once outside the loop instead of overwriting it with each readdir call. Redefine baselen as the length with that slash, and add dirlen for the length without it. The result is slightly less wasteful and can use the the cheaper strbuf_addstr instead of strbuf_addf without losing clarity.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 sha1_file.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
Show changes to sha1_file.c +7 −4
diff --git a/sha1_file.c b/sha1_file.c
index 5862386cd0..6a9deb9e68 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3749,7 +3749,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 				each_loose_subdir_fn subdir_cb,
 				void *data)
 {
-	size_t origlen, baselen;
+	size_t origlen, dirlen, baselen;
 	DIR *dir;
 	struct dirent *de;
 	int r = 0;
@@ -3760,7 +3760,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 	origlen = path->len;
 	strbuf_complete(path, '/');
 	strbuf_addf(path, "%02x", subdir_nr);
-	baselen = path->len;
+	dirlen = path->len;
 
 	dir = opendir(path->buf);
 	if (!dir) {
@@ -3770,12 +3770,15 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 		return r;
 	}
 
+	strbuf_addch(path, '/');
+	baselen = path->len;
+
 	while ((de = readdir(dir))) {
 		if (is_dot_or_dotdot(de->d_name))
 			continue;
 
 		strbuf_setlen(path, baselen);
-		strbuf_addf(path, "/%s", de->d_name);
+		strbuf_addstr(path, de->d_name);
 
 		if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2)  {
 			char hex[GIT_MAX_HEXSZ+1];
@@ -3801,7 +3804,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 	}
 	closedir(dir);
 
-	strbuf_setlen(path, baselen);
+	strbuf_setlen(path, dirlen);
 	if (!r && subdir_cb)
 		r = subdir_cb(subdir_nr, path->buf, data);
 

-- 
2.13.2
Jeff King· Jul 9, 2017, 11:00 UTC · re: René Scharfe · lore

Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()

On Sat, Jul 08, 2017 at 10:59:06AM +0200, René Scharfe wrote:
Show 5 quoted lines
> Add the slash between loose object subdirectory and file name just once
> outside the loop instead of overwriting it with each readdir call.
> Redefine baselen as the length with that slash, and add dirlen for the
> length without it.  The result is slightly less wasteful and can use the
> the cheaper strbuf_addstr instead of strbuf_addf without losing clarity.
This patch looks correct to me.

I'm a little lukewarm on it overall, though. I'd be shocked if the efficiency change is measurable. What I really care about is whether the result is easier to read or not.

On the plus side, this moves an invariant out of the loop. On the minus side, it has to introduce an extra variable for "length we add on to" versus "dir length to pass to the subdir_cb". That's not rocket science, but it does slightly complicate things (though I note we already have "origlen", so this is bumping us from 2 to 3 length variables, not 1 to 2).

So I dunno. It's fine with me if we take it, and fine if we leave it.
-Peff
René Scharfe· Jul 9, 2017, 13:26 UTC · re: Jeff King · lore

Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()

Am 09.07.2017 um 13:00 schrieb Jeff King:
Show 20 quoted lines
> On Sat, Jul 08, 2017 at 10:59:06AM +0200, René Scharfe wrote:
> 
>> Add the slash between loose object subdirectory and file name just once
>> outside the loop instead of overwriting it with each readdir call.
>> Redefine baselen as the length with that slash, and add dirlen for the
>> length without it.  The result is slightly less wasteful and can use the
>> the cheaper strbuf_addstr instead of strbuf_addf without losing clarity.
> 
> This patch looks correct to me.
> 
> I'm a little lukewarm on it overall, though. I'd be shocked if the
> efficiency change is measurable. What I really care about is whether the
> result is easier to read or not.
> 
> On the plus side, this moves an invariant out of the loop. On the minus
> side, it has to introduce an extra variable for "length we add on to"
> versus "dir length to pass to the subdir_cb". That's not rocket science,
> but it does slightly complicate things (though I note we already have
> "origlen", so this is bumping us from 2 to 3 length variables, not 1 to
> 2).

Admittedly this is more of an OCD thing. Overwriting a character 200 times or parsing a format string don't very long compared to the rest of the function, but it's a bit itchy.

René
Junio C Hamano· Jul 9, 2017, 16:41 UTC · re: Jeff King · lore

Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()

Jeff King <peff@peff.net> writes:
Show 22 quoted lines
> On Sat, Jul 08, 2017 at 10:59:06AM +0200, René Scharfe wrote:
>
>> Add the slash between loose object subdirectory and file name just once
>> outside the loop instead of overwriting it with each readdir call.
>> Redefine baselen as the length with that slash, and add dirlen for the
>> length without it.  The result is slightly less wasteful and can use the
>> the cheaper strbuf_addstr instead of strbuf_addf without losing clarity.
>
> This patch looks correct to me.
>
> I'm a little lukewarm on it overall, though. I'd be shocked if the
> efficiency change is measurable. What I really care about is whether the
> result is easier to read or not.
>
> On the plus side, this moves an invariant out of the loop. On the minus
> side, it has to introduce an extra variable for "length we add on to"
> versus "dir length to pass to the subdir_cb". That's not rocket science,
> but it does slightly complicate things (though I note we already have
> "origlen", so this is bumping us from 2 to 3 length variables, not 1 to
> 2).
>
> So I dunno. It's fine with me if we take it, and fine if we leave it.

Unlike origlen, base vs dir lengths are not strictly needed; we prepare the base including '/', and we know we always have just one '/' at the end, so anybody that uses dirlen to truncate it back to the original before passing it down can truncate to (baselen-1), no?

In other words, something like this (not an incremental but a replacement) to keep calling "baselen" the length of the leading constant part we append to?

 sha1_file.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
Show changes to sha1_file.c +5 −3
diff --git a/sha1_file.c b/sha1_file.c
index 5862386cd0..d277b32bf1 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3760,7 +3760,6 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 	origlen = path->len;
 	strbuf_complete(path, '/');
 	strbuf_addf(path, "%02x", subdir_nr);
-	baselen = path->len;
 
 	dir = opendir(path->buf);
 	if (!dir) {
@@ -3770,12 +3769,15 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 		return r;
 	}
 
+	strbuf_addch(path, '/');
+	baselen = path->len;
+
 	while ((de = readdir(dir))) {
 		if (is_dot_or_dotdot(de->d_name))
 			continue;
 
 		strbuf_setlen(path, baselen);
-		strbuf_addf(path, "/%s", de->d_name);
+		strbuf_addstr(path, de->d_name);
 
 		if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2)  {
 			char hex[GIT_MAX_HEXSZ+1];
@@ -3801,7 +3803,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 	}
 	closedir(dir);
 
-	strbuf_setlen(path, baselen);
+	strbuf_setlen(path, baselen - 1); /* chomp the '/' that we added */
 	if (!r && subdir_cb)
 		r = subdir_cb(subdir_nr, path->buf, data);
Jeff King· Jul 10, 2017, 00:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()

On Sun, Jul 09, 2017 at 09:41:51AM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> > On the plus side, this moves an invariant out of the loop. On the minus
> > side, it has to introduce an extra variable for "length we add on to"
> > versus "dir length to pass to the subdir_cb". That's not rocket science,
> > but it does slightly complicate things (though I note we already have
> > "origlen", so this is bumping us from 2 to 3 length variables, not 1 to
> > 2).
> >
> > So I dunno. It's fine with me if we take it, and fine if we leave it.
> 
> Unlike origlen, base vs dir lengths are not strictly needed; we
> prepare the base including '/', and we know we always have just one
> '/' at the end, so anybody that uses dirlen to truncate it back to
> the original before passing it down can truncate to (baselen-1), no?
> 
> In other words, something like this (not an incremental but a
> replacement) to keep calling "baselen" the length of the leading
> constant part we append to?

Yeah, I think that is correct. And you could even drop origlen by replacing it with "baselen - 3" at the end. But somehow doing the computation on the fly actually seems more complicated to me (from the perspective of a reader who is trying to make sure all is correct).

-Peff
Junio C Hamano· Jul 10, 2017, 02:10 UTC · re: Jeff King · lore

Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()

Jeff King <peff@peff.net> writes:
> ... And you could even drop origlen by
> replacing it with "baselen - 3" at the end. But somehow doing the
> computation on the fly actually seems more complicated to me (from the
> perspective of a reader who is trying to make sure all is correct).

True enough. I personally do not mind any of the three variants (including the original) and would rather not spend too much time micro-optimizing this codepath, lest the next clever person starts to turn the loop in for_each_loose_file_in_objdir() to a nested loop that runs 16 times each to avoid copying the same leading byte again and again ;-)

René Scharfe· Jul 12, 2017, 17:58 UTC · re: Junio C Hamano · lore

Re: [PATCH] sha1_file: add slash once in for_each_file_in_obj_subdir()

Am 10.07.2017 um 04:10 schrieb Junio C Hamano:
Show 13 quoted lines
> Jeff King <peff@peff.net> writes:
> 
>> ... And you could even drop origlen by
>> replacing it with "baselen - 3" at the end. But somehow doing the
>> computation on the fly actually seems more complicated to me (from the
>> perspective of a reader who is trying to make sure all is correct).
> 
> True enough.  I personally do not mind any of the three variants
> (including the original) and would rather not spend too much time
> micro-optimizing this codepath, lest the next clever person starts
> to turn the loop in for_each_loose_file_in_objdir() to a nested loop
> that runs 16 times each to avoid copying the same leading byte again
> and again ;-)
*evil grin*

Anyway, I agree that this patch is not worth spending much time on. I still think the result is cleaner (perhaps due to my strbuf_addf allergy), though it's longer and perhaps adding a twelfth variable makes the function too complicated. The magic chomping variant is a clever compromise -- but needs a comment to explain itself. So if you are indifferent please just drop it.

Thanks, René

← back to recent threads