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

rTorrent: Refactor source code #1014

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

stickz
Copy link
Contributor

@stickz stickz commented May 16, 2023

This pull request refactors the rTorrent source code for faster installation, better logging readability and reduced maintenance. Five single threaded tasks are executed in parallel. When completed, swizzin.log is updated with dividing sections for each task. Package installation is told to wait with less than 4 threads, otherwise it proceeds at the same time to consume idle resources.

  • Reduced initial install times by 30s on X86 and 60s on ARM64
  • Reduced upgrade times by 25s on X86 and 50s on ARM64
  • Improved readability of swizzin.log by separating different tasks.
  • Reduced maintenance of code using variables for paths, versions etc.

This commit allows modular and parallel building of rTorrent dependencies. Output is appended to `swizzin.log` afterwards more cleanly than before. Temporary log files are used to prevent race conditions from happening. Threads counts and task concurrency are limited to ensure stable installation times.
@stickz
Copy link
Contributor Author

stickz commented May 30, 2023

This pull request has been updated.

  • The build process no longer uses hardcoded paths for the xmlrpc, libtorrent and rtorrent temp folders.
  • Fixed a bug causing temporary error files to be left over from the preparation processes.

The rtorrent removal stack code is now "fixed" and "refactored".

  • Package removal now uses the apt_remove wrapper.
  • Reduced upgrade time by 10s by including binary removal in the preparation processes.
  • xmlrpc and libtorrent are now removed properly for box install, upgrade and remove.

@liaralabs
Copy link
Member

I have been mulling over the async processes and I have decided not to move forward with them. I understand that it saves time, but ultimately it's a layer of complexity that I don't want merged into the scripts. The trade-off (saving some time during tasks which are not frequently run) is not worth the increased complexity and other potential issues that might result from this.

There are other improvements in here worth merging, but ultimately, foregoing the forking and background tasks.

@stickz
Copy link
Contributor Author

stickz commented Jun 12, 2023

I have been mulling over the async processes and I have decided not to move forward with them. I understand that it saves time, but ultimately it's a layer of complexity that I don't want merged into the scripts... and other potential issues that might result from this.

Could we consider implementing low complexity async tasks, with the only acceptable risk for potential issues being logging? The trade-off between added logging complexity and time-saved is worth it IMHO. It would allow level 3 compile optimizations and take minutes off certain tasks such as dhparm generation on openssl for nginx. wait is 100% safe. It waits for failure or success.

For instance, the only shared code and added complexity for mktorrent and libudns is logging. The only potential issue is for the logging output not to be copied to swizzin.log properly. We are building a separate .so extension, without forking information between threads, with the exception of the file path for the temporary log file; so we can append it afterwards. We already have checks in place on GitHub, to ensure the separate shell script is executable. This is not a potential script issue.

@github-actions github-actions bot added the has conflicts This PR has conflicts against master label Jul 16, 2023
@stickz stickz marked this pull request as draft August 2, 2023 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has conflicts This PR has conflicts against master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants