Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add cached eval #56

Merged
merged 3 commits into from
Jul 25, 2020
Merged

add cached eval #56

merged 3 commits into from
Jul 25, 2020

Conversation

casperdcl
Copy link
Collaborator

@casperdcl casperdcl commented Jul 9, 2020

Attempts to cache runs of underlying commands so that e.g. #{gpu_bg_color}#{gpu_percentage} only calls nvidia-smi once per update.

Notes:

  • runs cached commands a maximum of once every 2 seconds
  • caches based on date +%s and basename command, ignoring command arguments. This could be a problem if running the same command with different arguments (not currently a problem)

details

While testing this, I think I've found an upstream problem (with tmux or tpm). Basically

set -g status-right "%H:%M:%S #(sleep 1)"
set -g status-interval 5

will ignore status-interval and result in one update per second (!) which appears to be #15.

  1. status-interval not respected #15 (status-interval not respected) looks like an upstream bug
  2. each update, commands may be run in parallel? e.g. set -g status-right #(write_hello_to_debug_file) #(write_bye_to_debug_file) won't guarantee hello is written before bye. This means that caching within an update may not work. Needs investigating... it's possible that some higher level code (outside this repo) handles caching of #(external commands)
  3. oddly, before this PR, I get updates more than 1/s, but after it's about once every 5 sec, respecting status-interval. This seems to fix (maybe) status-interval not respected #15 but no there's not clear reason why... I suspect some higher level logic may be doing something like:
    commands = find_pattern("#(*)", status_right)
    while True:
        execution_time = exec(commands)
        if execution_time < 1:
            sleep(status_interval)

debugging

in ~/.tmux.conf:

set -g status-right '#{cpu_bg_color}#{cpu_percentage}#[bg=green][#{ram_bg_color}#{ram_percentage}#[bg=green]] #{gpu_bg_color}#{gpu_percentage}#[bg=green][#{gram_bg_color}#{gram_percentage}#[bg=green]]'

print to a debug file in ~/.tmux/plugins/tmux-cpu/debug.log:

diff --git a/scripts/helpers.sh b/scripts/helpers.sh
index df31cfb..d1e3e84 100644
--- a/scripts/helpers.sh
+++ b/scripts/helpers.sh
@@ -99,7 +99,9 @@ cached_eval(){
   local val="$(get_cache_val "$key")"
   if [ -z "$val" ]; then
     put_cache_val "$key" "$($command "${@:2}")"
+    echo miss "$key" >> ~/.tmux/plugins/tmux-cpu/debug.log
   else
     echo -n "$val"
+    echo hit "$key" >> ~/.tmux/plugins/tmux-cpu/debug.log
   fi
 }
  • observation: will print miss nvidia-smi 4 times in quick succession each update
  • expected: miss nvidia-smi once, then hit nvidia-smi 3 times
tail -f ~/.tmux/plugins/tmux-cpu/debug.log

- fixes #27
- related #15
- related #5
@ctjhoa
Copy link
Member

ctjhoa commented Jul 13, 2020

Sorry, I have very few times to work on my open source projects. I'll try to review your work tomorrow.

Copy link
Member

@ctjhoa ctjhoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 comment.
It looks good to me otherwise

scripts/helpers.sh Outdated Show resolved Hide resolved
scripts/helpers.sh Outdated Show resolved Hide resolved
scripts/helpers.sh Outdated Show resolved Hide resolved
local key="$1"
local val="${@:2}"
local tmpdir="$(get_tmp_dir)"
[ ! -d "$tmpdir" ] && mkdir -p "$tmpdir" && chmod 0700 "$tmpdir"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chmod 0700 is not wanted on global system tmp directory

Copy link
Collaborator Author

@casperdcl casperdcl Jul 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's never global. get_tmp_dir always returns an EUID-specific subdir.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one of the main reasons I looked at tmux-copycat's implementation for reference is because they would've fixed any problems with it by now if it was wrong.

@@ -62,3 +62,46 @@ command_exists() {
local command="$1"
command -v "$command" &> /dev/null
}

get_tmp_dir() {
local tmpdir="${TMPDIR:-${TMP:-${TEMP:-/tmp}}}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From man tmux

tmux stores the server socket in a directory under TMUX_TMPDIR or /tmp if it is unset.

So either TMUX_TMPDIR or /tmp should exist

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe that's why tmux-copycat does that if inside tmux env TMUX_TMPDIR turn into TMPDIR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the back and forth

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe; though I don't really see much point in reverting - surely checking TMP & TEMP is fine?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with both solution

@casperdcl casperdcl merged commit 20120a3 into master Jul 25, 2020
@casperdcl casperdcl deleted the cached_eval branch July 25, 2020 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update overhead/efficiency
2 participants