pystardust / ani-cli

A cli tool to browse and play anime
GNU General Public License v3.0
7.63k stars 533 forks source link

Poor Performance on Windows After Commit #bd597...fff1be #1157

Closed the-real-ale closed 1 year ago

the-real-ale commented 1 year ago

Metadata (please complete the following information) Version: v4.4.3 OS: Windows 10 Shell: git bash in windows terminal preview

Describe the bug Extremely slow on Windows 10 at "fetching links" stage on and after commit bd597a20230009c25cf5b9a3d0656cf1c2fff1be

Commit 26b98b4c019864a9d2e5882dab95fdb25af256de (the previous commit) works as expected.

Steps To Reproduce

  1. git checkout 26b98b4c019864a9d2e5882dab95fdb25af256de
  2. ./ani-cli [cory in the house]
  3. Observe "fetching links" speed. Should take about 1 second.
  4. git checkout bd597a20230009c25cf5b9a3d0656cf1c2fff1be
  5. ./ani-cli [cory in the house]
  6. "fetching links" stage is ~2 minutes or more (this seems to depend on system specs too)

Expected behavior Fetching links should take a few seconds before episode plays.

justchokingaround commented 1 year ago

what terminal emulator are you using? windows' terminal, git bash?

the-real-ale commented 1 year ago

what terminal emulator are you using? windows' terminal, git bash?

Using git bash through windows terminal preview

ttx99 commented 1 year ago

last time I also used it was also so slow but I assume it was the old laptop just having 4gb ram and im not that familiar with windows 10

justchokingaround commented 1 year ago

try using a different terminal emulator. this might be caused by fzf

the-real-ale commented 1 year ago

try using a different terminal emulator. this might be caused by fzf

I think it's the hex_to_ascii function. I get good performance by replacing the provider ID code with xxd:

    bin=$(echo "$provider_id" | xxd -r -p)
    provider_id="$(printf "%s" "$bin" | sed "s/\/clock/\/clock\.json/")"
justchokingaround commented 1 year ago

xxd is a dependency that comes preinstalled with vim, so it's available on most distros by defaults, but it is not a core-util so we chose to not use it

the-real-ale commented 1 year ago

Understood, I did a quick swap to check if that's the bottleneck. I don't know what a good solution is.

justchokingaround commented 1 year ago

can you run the script in debug mode, to see exactly where in the hex_to_ascii function it slows down/freezes please? that would be helpful for me to improve it

sh -x $(command -v ani-cli)

or if you are running a local file and not the one installed in path:

sh -x ani-cli
the-real-ale commented 1 year ago

Yes, here's the output of sh -x ani-cli "fullmetal alchemist: brotherhood" 2> log.txt I have to paste it as a file because it loops at the hex stage about 7000 times. log.txt

the-real-ale commented 1 year ago

I think I have a solution for this using only sed and printf

> printf $(echo 676F7474612074657374 | sed -e 's/../\\x&/g')
> gotta test

I'll test this out once #1165 is merged and submit the change if it fixes the performance.

CoolnsX commented 1 year ago

I think I have a solution for this using only sed and printf

> printf $(echo 676F7474612074657374 | sed -e 's/../\\x&/g')
> gotta test

I'll test this out once #1165 is merged and submit the change if it fixes the performance.

noice, checking if it's posix compliant and works on mac

CoolnsX commented 1 year ago

sorry, it's not posix compliant, it only works in bash and zsh image

the-real-ale commented 1 year ago

sorry, it's not posix compliant, it only works in bash and zsh

Alright, thanks for testing.

CoolnsX commented 1 year ago

however, I found a way that makes it faster, by 50%

CoolnsX commented 1 year ago

thank you for giving me push to actually activate my neurons

CoolnsX commented 1 year ago

@the-real-ale pushed the new code, can you test on windows?

CoolnsX commented 1 year ago

reason why ani-cli is slow on windows

https://github.com/pystardust/ani-cli/assets/76195824/86ceb63d-ec2f-4ccf-90d9-f33643e0e68e

CoolnsX commented 1 year ago

it takes 10 seconds for that conversion to complete in windows, in linux it's within 100ms

the-real-ale commented 1 year ago

Right, I think git-bash specifically is not optimal for this kind of thing. Probably why the xxd solution works so well here. I tested the latest hex_decryption and I do see a speed improvement of ~30% - ~50%

CoolnsX commented 1 year ago

Now the thing is xxd won't work anymore, cause there is encryption going on and instead of going like hex_to_ascii then decryption, it's now direct decryption

CoolnsX commented 1 year ago

So the O(n) + O(n) loops became only O(n)

the-real-ale commented 1 year ago

I found more detail in this stackexchange answer. Calling to external processes in Windows is just inherently slow. This may give a hint at how to optimize the script, but it would require a lot of work, so I'm going to close this bug as the problem is inherently with mingw/cygwin bash and Windows.

CoolnsX commented 1 year ago

@the-real-ale anyway here is a C program I wrote in a jiffy to do the job of decryption, I am not gud at it ..

#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[]) {
    if (argc != 2) {
        printf("Usage: ./decrypt <hex_string>\n");
        return 1;
    }
    char *hex_string = argv[1];
    while (*hex_string) {
        char hex[3] = {hex_string[0], hex_string[1], '\0'};
        hex_string += 2;
        printf("%c",(int) strtol(hex, NULL, 16) ^ 48);
    }
    return 0;
}

you can compile it and pass the hex as args then it will output the string, I benchmarked it and it's giving output in 1-2 ms on linux, since it's a c program it won't be affected by that fork mechanism (maybe?).

the-real-ale commented 1 year ago

@CoolnsX Nice, I was thinking of making one in rust :) Much appreciated!

CoolnsX commented 1 year ago

tested on git bash in windows, c program takes within 80 ms

the-real-ale commented 1 year ago

Hey sorry for the spam, but I found another workaround. Both WSL1 and WSL2 are quicker than git bash on windows. Install requirements as you would normally for ani-cli on the linux vm, install mpv on Windows host, and then perform one extra step:

  1. create file /usr/local/bin/mpv or /usr/bin/mpv that allows the vm to call the host installation of mpv
    
    #!/bin/bash

mpv.exe "$@"


and make sure this mpv passthrough script is executable.

This way, the script is not bogged down by the cygwin bash forking issue, and ani-cli still plays the stream properly on the host mpv.
CoolnsX commented 1 year ago

Don't worry it's not spam as long as someone find it helpful.