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

Multithreading with Runspaces #150

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Carterpersall
Copy link
Contributor

@Carterpersall Carterpersall commented Sep 1, 2022

  • Uses runspaces to speed up execution by making winfetch multithreaded

TODO:

  • Implement runspaces for the rest of the info functions
  • Maybe: Add config option to disable runspace implementation
  • Figure out what is going on with info_memory and info_battery within runspaces
    • Fix it

- Made info_disk function activate in another thread in an attempt to decrease lag caused by it
- Should be "*easily*" expandable to include entire script for large performance gain
@Carterpersall
Copy link
Contributor Author

Carterpersall commented Sep 1, 2022

Performance with my config (Default with a few options disabled):

  • PowerShell 5:

    • With Runspaces:
    image
    • Without:
    image
  • PowerShell 7 (Takes longer to launch, but winfetch executes faster)

    • With Runspaces:
    image
    • Without:
    image

Performance with runspaces and just disks enabled in config is slightly slower due to not being able to execute during the other operations' runtimes; this is the only case that I know of though there runspaces would be slower so IMO a slight decrease in performance in a single edge case for a significant uplift in all others is a good tradeoff.

@rashil2000
Copy link
Member

Please don't make changes to the function definition (revert changes on line 801 and 829).

How will you implement this for the rest of the functions?

 - Made the runspaces implementation expandable
  - Implemented runspace pools which speeds up multiple runspaces significantly
    - Single runspace slightly faster too
  - Added array in which a runspace is created for each entry that is enabled in the config
    - Allows for some entries which may be faster without runspaces to be executed on the main thread
@Carterpersall
Copy link
Contributor Author

Carterpersall commented Sep 4, 2022

Please don't make changes to the function definition (revert changes on line 801 and 829).

The change made on line 801 is a temporary fix to an issue that was occurring and will be reverted when I fix it

The change on line 829 was made to prevent me from having to pass get_level_info along with the variables and function it depends on into the runspace, which I think would be wasteful due to its use of the function being minimal and it not using the function or variables it depends on. Instead, I took the single part of the function it used and directly implemented it where the function declaration was. This also likely saves a little bit of performance by bypassing all of the unnecessary checks the function performs.

How will you implement this for the rest of the functions?

As seen in the commit I just made, I implemented runspace pools and created a loop that will create and execute the runspaces which will benefit from being multithreaded (some functions likely will be faster outside of runspaces due to their speed and the overhead of runspaces).

I will convert the rest of the functions for runspaces likely just like how I did for disks:

  1. Convert Function into scriptblock
  2. Instead use the function for the retrieval of the runspace's output; so when the print loop executes it, the output is likely already ready, and if not, it'll wait until it is done (likely can be optimized, but that is for a later date)
  3. Tweak the scriptblocks to fix any bugs that show up

@rashil2000
Copy link
Member

The change on line 829 was made to prevent me from having to pass get_level_info along with the variables and function it depends on into the runspace, which I think would be wasteful due to its use of the function being minimal and it not using the function or variables it depends on.

No, you have ignored the variable $diskstyle, which is the user's preference for displaying percentages.

I will convert the rest of the functions for runspaces likely just like how I did for disks:

  1. Convert Function into scriptblock
  2. Instead use the function for the retrieval of the runspace's output; so when the print loop executes it, the output is likely already ready, and if not, it'll wait until it is done (likely can be optimized, but that is for a later date)
  3. Tweak the scriptblocks to fix any bugs that show up

To prevent rewriting major parts of the code, you can directly use a function's definition (by assigning $function:info_disk, for example). Now, while collecting output, if runspaces are disabled in the config file, you can call info_disk directly (as done below), https://github.com/kiedtl/winfetch/blob/89a4524fc3cc715df9be373a111c7a6642018efa/winfetch.ps1#L1059 and if runspaces are enabled, use the job waiting method as done in your PR currently.

@Carterpersall
Copy link
Contributor Author

Carterpersall commented Sep 4, 2022

No, you have ignored the variable $diskstyle, which is the user's preference for displaying percentages.

Oh, I seem to have missed that. I'll see if I can add that in or just pass the function and variables in.

To prevent rewriting major parts of the code, you can directly use a function's definition (by assigning $function:info_disk, for example). Now, while collecting output, if runspaces are disabled in the config file, you can call info_disk directly (as done below),

https://github.com/kiedtl/winfetch/blob/89a4524fc3cc715df9be373a111c7a6642018efa/winfetch.ps1#L1059

and if runspaces are enabled, use the job waiting method as done in your PR currently.

Yeah, that might work. I'll see what I can whip up when I have the time

Also, progress might be slow due to me being busy due to college. But I intend to get this done as I use this program all the time

@rashil2000
Copy link
Member

Also, progress might be slow due to me being busy due to college. But I intend to get this done as I use this program all the time

Sure, no worries. This is a pretty significant change, so please take all the time you need :)

- Created $Vars and $Funcs
  - Contains all functions and variables the runspaces might need
- Instead of having a scriptblock and helper info function, the original info functions are converted to a scriptblock and the helper function was moved into the $info loop
  - Functionally the same while preserving the original functionality of the script

- Added config option and parameter $runspaces where if enabled(default), runspaces are used. And if disabled, runspaces aren't used.
  - Disabling runspaces is very useful for debugging as step through in PowerShell ISE doesn't show operations within a runspace.
@Carterpersall
Copy link
Contributor Author

Carterpersall commented Sep 6, 2022

To prevent rewriting major parts of the code, you can directly use a function's definition (by assigning $function:info_disk, for example). Now, while collecting output, if runspaces are disabled in the config file, you can call info_disk directly (as done below),

https://github.com/kiedtl/winfetch/blob/89a4524fc3cc715df9be373a111c7a6642018efa/winfetch.ps1#L1059

and if runspaces are enabled, use the job waiting method as done in your PR currently.

Implemented in the last commit.
I will work on passing arguments and functions into the runspaces now

- Adds $Vars and $Funcs into the runspace
- Adds helper script into the beginning of the runspaces to declare needed variables and functions
  - Currently declares all of the variables and functions for every runspace, which is inefficient. Could be solved with much increased complexity if performance issues arise from it.
- Reverted changes to info_disk as the issues were solved by passing variables in
@Carterpersall
Copy link
Contributor Author

Took a bit of work, but I figured it out eventually.

Please don't make changes to the function definition (revert changes on line 801 and 829).

Reverted in the most recent commit

Theoretically, all I need to do now is start adding functions to $RunspaceOps and they'll hopefully just work. Probably will run into an issue somewhere but I guess one can hope

- Fixed bug on line 1160
- Closes and disposes of runspaces and the pool as it is good practice to prevent issues and conflicts
@Carterpersall
Copy link
Contributor Author

It seems that the only functions with issues are memory and battery. If I can't figure out what's wrong, I'll just have them disabled by default in $RunspaceOps. I'll post my final speeds soon

@Carterpersall
Copy link
Contributor Author

Carterpersall commented Sep 6, 2022

Speeds:
Note: This is on my 16-thread laptop CPU downclocked to 1.06 GHz, which is pretty much the best-case scenario for runspaces (slow single core, many cores)

Everything Enabled:

  • PowerShell 5:

    • With Runspaces:
      image

    • Without Runspaces:
      image

  • PowerShell 7:

    • With Runspaces:
      image

    • Without Runspaces:
      image

Everything Enabled Except "ps_pkgs", "cpu_usage", and "locale":

  • PowerShell 5:

    • With Runspaces:
      image

    • Without Runspaces:
      image

  • PowerShell 7:

    • With Runspaces:
      image

    • Without Runspaces:
      image

Nothing Enabled:

  • PowerShell 5:

    • With Runspaces:
      image

    • Without Runspaces:
      image

  • PowerShell 7:

    • With Runspaces:
      image

    • Without Runspaces:
      image

Defaults:

  • PowerShell 5:

    • With Runspaces:
      image

    • Without Runspaces:
      image

  • PowerShell 7:

    • With Runspaces:
      image

    • Without Runspaces:
      image

My Config:

  • PowerShell 5:

    • With Runspaces:
      image

    • Without Runspaces:
      image

  • PowerShell 7:

    • With Runspaces:
      image

    • Without Runspaces:
      image

@Carterpersall
Copy link
Contributor Author

Carterpersall commented Sep 7, 2022

In terms of speed, running with runspaces is beneficial in almost every configuration except those where one has every 'slow' operation disabled. When only near-instant operations are enabled, the slight overhead of runspaces overpowers the speed increase of parallelization, leading to a slight performance decrease.

So runspaces cause a larger performance increase the longer winfetch takes to run but also causes a small performance decrease if it runs fast enough. I think most people will see either a performance increase or an almost unnoticeable performance decrease, which I think is enough to keep the default as enabled.

- Set $RunspaceOps to every operation that doesn't error
  - Having memory and battery enabled cause really weird issues sometimes
@Carterpersall
Copy link
Contributor Author

Carterpersall commented Sep 8, 2022

I think I have figured out what is causing the issues with the info_memory and info_battery functions.

It seems that for some reason when a call to get_level_info exists within those two functions, powershell seemingly "forgets" some combination of these things:

  1. $Job
  2. truncate_line
  3. Test-Path
  4. Write-Output

The quantity of errors and when they occur seems to be random, except that they all occur from line 1195 down. Any execution of the function causes the issue, even by calling the function using & $function:get_level_info
My guess is that if the issue triggers, it triggers when the function is called within the runspace, contributing to the randomness of the point at which the errors begin.

- Added some comments and tweaked some existing ones
- Removed $t from $Vars as it was unused
@Carterpersall
Copy link
Contributor Author

I think it's best to keep memory and battery out of runspace execution for now as it seems that fixing their issues would require a lot more work than it's worth.

@Carterpersall Carterpersall marked this pull request as ready for review September 8, 2022 19:35
@Carterpersall
Copy link
Contributor Author

@rashil2000 Unless you have any recommendations, I think the pull request is ready to be merged.

@rashil2000
Copy link
Member

Okay, will review it in some time

@Carterpersall Carterpersall mentioned this pull request Sep 13, 2022
16 tasks
@Carterpersall Carterpersall mentioned this pull request Sep 30, 2022
4 tasks
@Carterpersall Carterpersall marked this pull request as draft February 7, 2023 21:52
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.

2 participants