I believe the first two should be ok, but I'm not sure what I myself
think of the third one. Perhaps the saving is not worth the
complexity, but it does annoy my optimization nerve to see all the
unnecessary duplicated work being done.
Rasmus Villemoes (3):
grep: move grep_source_init outside critical section
grep: simplify grep_oid and grep_file
grep: avoid one strdup() per file
builtin/grep.c | 25 ++++++++++++-------------
grep.c | 8 ++++++--
2 files changed, 18 insertions(+), 15 deletions(-)
--
2.15.1
grep_source_init typically does three strdup()s, and in the threaded
case, the call from add_work() happens while holding grep_mutex.
We can thus reduce the time we hold grep_mutex by moving the
grep_source_init() call out of add_work(), and simply have add_work()
copy the initialized structure to the available slot in the todo
array.
This also simplifies the prototype of add_work(), since it no longer
needs to duplicate all the parameters of grep_source_init(). In the
callers of add_work(), we get to reduce the amount of code duplicated in
the threaded and non-threaded cases slightly (avoiding repeating the
"GREP_SOURCE_OID, pathbuf.buf, path, oid" argument list); a subsequent
cleanup patch will make that even more so.
Signed-off-by: Rasmus Villemoes <redacted>
---
builtin/grep.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
There is only one instance of grep_source_init(GREP_SOURCE_FILE), and in
that case the path and identifier arguments are equal - not just as
strings, but the same pointer is passed. So we can save some time and
memory by reusing the gs->path = xstrdup_or_null(path) we have already
done as gs->identifier, and changing grep_source_clear accordingly
to avoid a double free.
Signed-off-by: Rasmus Villemoes <redacted>
---
grep.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
In the NO_PTHREADS or !num_threads case, this doesn't change
anything. In the threaded case, note that grep_source_init duplicates
its third argument, so there is no need to keep [path]buf.buf alive
across the call of add_work().
Signed-off-by: Rasmus Villemoes <redacted>
---
builtin/grep.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
From: Brandon Williams <hidden> Date: 2018-02-15 22:02:20
On 02/15, Rasmus Villemoes wrote:
I believe the first two should be ok, but I'm not sure what I myself
think of the third one. Perhaps the saving is not worth the
complexity, but it does annoy my optimization nerve to see all the
unnecessary duplicated work being done.
I agree, the first two seem like good changes to me though I don't know
if i like the complexity that the third introduces.
From: Jeff King <hidden> Date: 2018-02-15 22:17:20
On Thu, Feb 15, 2018 at 10:56:13PM +0100, Rasmus Villemoes wrote:
grep_source_init typically does three strdup()s, and in the threaded
case, the call from add_work() happens while holding grep_mutex.
We can thus reduce the time we hold grep_mutex by moving the
grep_source_init() call out of add_work(), and simply have add_work()
copy the initialized structure to the available slot in the todo
array.
This also simplifies the prototype of add_work(), since it no longer
needs to duplicate all the parameters of grep_source_init(). In the
callers of add_work(), we get to reduce the amount of code duplicated in
the threaded and non-threaded cases slightly (avoiding repeating the
"GREP_SOURCE_OID, pathbuf.buf, path, oid" argument list); a subsequent
cleanup patch will make that even more so.
I think this makes sense. It does blur the memory ownership lines of the
grep_source, though. Can we make that more clear with a comment here:
like:
/* leak grep_source, whose fields are now owned by add_work() */
or something? We could even memset() it back to all-zeroes to avoid an
accidental call to grep_source_clear(), but that's probably unnecessary
if we have a comment.
-Peff
From: Jeff King <hidden> Date: 2018-02-15 22:18:44
On Thu, Feb 15, 2018 at 10:56:15PM +0100, Rasmus Villemoes wrote:
There is only one instance of grep_source_init(GREP_SOURCE_FILE), and in
that case the path and identifier arguments are equal - not just as
strings, but the same pointer is passed. So we can save some time and
memory by reusing the gs->path = xstrdup_or_null(path) we have already
done as gs->identifier, and changing grep_source_clear accordingly
to avoid a double free.
IMHO this special case is not really worth it, unless we can show that
those few bytes saved somehow make a measurable difference in either
peak memory or execution speed.
-Peff
Changes in v2:
- Drop patch 3 with dubious gain/complexity ratio
- Add comments regarding ownership of grep_source
I was a little torn between copy-pasting the comment or just saying
"see above" in the second case. I think a memset would be confusing,
at least unless one extends the comment to explain why one then does
the memset despite the first half of the comment.
Rasmus Villemoes (2):
grep: move grep_source_init outside critical section
grep: simplify grep_oid and grep_file
builtin/grep.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
--
2.15.1
In the NO_PTHREADS or !num_threads case, this doesn't change
anything. In the threaded case, note that grep_source_init duplicates
its third argument, so there is no need to keep [path]buf.buf alive
across the call of add_work().
Signed-off-by: Rasmus Villemoes <redacted>
---
builtin/grep.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
grep_source_init typically does three strdup()s, and in the threaded
case, the call from add_work() happens while holding grep_mutex.
We can thus reduce the time we hold grep_mutex by moving the
grep_source_init() call out of add_work(), and simply have add_work()
copy the initialized structure to the available slot in the todo
array.
This also simplifies the prototype of add_work(), since it no longer
needs to duplicate all the parameters of grep_source_init(). In the
callers of add_work(), we get to reduce the amount of code duplicated in
the threaded and non-threaded cases slightly (avoiding repeating the
long "GREP_SOURCE_OID, pathbuf.buf, path, oid" argument list); a
subsequent cleanup patch will make that even more so.
Signed-off-by: Rasmus Villemoes <redacted>
---
builtin/grep.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
From: Jeff King <hidden> Date: 2018-02-23 18:01:15
On Fri, Feb 23, 2018 at 03:47:55PM +0100, Rasmus Villemoes wrote:
Changes in v2:
- Drop patch 3 with dubious gain/complexity ratio
- Add comments regarding ownership of grep_source
I was a little torn between copy-pasting the comment or just saying
"see above" in the second case. I think a memset would be confusing,
at least unless one extends the comment to explain why one then does
the memset despite the first half of the comment.
This looks good to me. Thanks for following up.
-Peff