make TaskWarriorShellout() do task --version only once - #159
Open
smemsh wants to merge 1 commit into
Open
Conversation
TaskWarriorShellout() is doing at least 3 task --version to get the instance (before we can run any methods), and then is doing these calls again for each filter_task(), task_add(), or sync() method call.TaskWarriorShellout() do task --version only onceryneeverett
commented
May 16, 2022
Contributor
Don't we still need to wrap the result in Wouldn't it be more readable to just cache the |
smemsh
commented
May 16, 2022
Author
Yes, I had missed that when copying the block, thanks.
Yes, agree that it's simpler, not sure why I didn't use that approach as it's the obvious way. I'll try it like that instead, thanks! |
smemshforce-pushed
the
task-version-once
branch
2 times, most recently
from
May 16, 2022 05:40
9b994af to
5ac48ddComparesmemsh
commented
May 16, 2022
Author
@ryneeverett seems to work. Hopefully it will be merged, perhaps someday ;-) |
ryneeverett
approved these changes
May 16, 2022
A TaskWarriorShellout instance calls .get_version() already thrice: once when determining if we .can_use() TaskWarriorShellout rather than TaskWarriorDirect, and then twice in the instance constructor itself. Therefore creation of the instance already calls it thrice. Each of these ends up executing "task --version" with execve(2) via subprocess.Popen(). Use of the methods .sync() or .filter_tasks() would execute this once again, while using .task_add() would do it twice. We therefore end up executing "task --version" four times for a single filter_tasks() and five times for a task_add(). Scripts which are doing multiple filters are constantly adding the overhead of these fork/execve just to obtain the version. My own personal taskgrep wrapper is making several calls with different filters, and strace is showing 18 different "task --version" calls, which on my modern-ish laptop adds about a tenth of a second to my taskgreps: $ TIMEFORMAT='time: %3Rs'; time \ for ((i = 0; i < 18; i++)) do task --version &>/dev/null; done time: 0.089s It's not huge, but these unncessary repeated "task --version" calls really can just be done once and the value cached as a class attribute Note that we cannot do this in the instance constructor, because can_use() is a class method called at module import time, before any instance will have been created, so we need get_version() to continue to be a class method. Thanks also to @ryneeverett for suggesting to use a simple cached class attribute in get_version() rather than the first implementation which moved these statements into the class definition.
bergercookie added a commit
to bergercookie/taskw-ng
that referenced
this pull request
Jan 23, 2024
Merge PR ralphbean/taskw#159 of original taskw repo
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TaskWarriorShellout()is doing at least 3task --versionto get the instance (before we can run any methods), and then is doing these calls again for eachfilter_task(),task_add(), orsync()method call.This patch moves
task --versionexecution to a cached class attribute so it can be done one time only during the lifetime.Note that we cannot move this to instance constructor because it's run during module import at toplevel (via
.can_use()call at bottom oftaskw/warrior.py)Fixes#158
Thanks also to @ryneeverett for suggesting use a cached class attribute in get_version()