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

feat: auto-retry with single-threaded writer on OOM #253

Open
3 tasks done
Ushie opened this issue Oct 10, 2023 · 8 comments
Open
3 tasks done

feat: auto-retry with single-threaded writer on OOM #253

Ushie opened this issue Oct 10, 2023 · 8 comments
Labels
Feature request Requesting a new feature that's not implemented yet Waiting on author Further information is requested

Comments

@Ushie
Copy link
Member

Ushie commented Oct 10, 2023

Type

Functionality

Issue

When running on limited memory, it may not be enough to patch a specific app and ends up throwing an OOM as seen in #193

Feature

ReVanced Patcher handles the OOM and restarts with single thread at the appropriate step

Motivation

ReVanced Patcher supports both single and multi-threads, multi-threads are preferred at all times as the patching process will overall be faster

This moves the hassle from the end-user to ReVanced Patcher, hopefully for a seamless transition with a few additional logs to state what happened

Additional context

The reason I'm opening this in the ReVanced Patcher repository instead of ReVanced Library or expecting it to be done by the clients is that hopefully, ReVanced Patcher can restart ONLY the necessary section instead of having to start over from the beginning

Acknowledgements

  • I have searched the existing issues and this is a new and no duplicate or related to another open issue.
  • I have written a short but informative title.
  • I filled out all of the requested information in this issue properly.
@Ushie Ushie added the Feature request Requesting a new feature that's not implemented yet label Oct 10, 2023
@Ushie Ushie changed the title feat: auto-retry with multi-threaded writer on OOM feat: auto-retry with single-threaded writer on OOM Oct 10, 2023
@LisoUseInAIKyrios
Copy link
Contributor

Perhaps patcher could also check how much memory is available, and if it's below some preset threshold then always use single threaded from the start.

@oSumAtrIX
Copy link
Member

@LisoUseInAIKyrios It depends on the app. Some apps can be patched with 500MB, such as YouTube with multiple threads, but TikTok requires 500MB with a single line due to having way more classes.

@LisoUseInAIKyrios
Copy link
Contributor

LisoUseInAIKyrios commented Oct 10, 2023

Then could alternatively check how many classes/methods are total, or the total file size of the original class/dex files, and use a threshold based on that. Then compare what YouTube and TikTok size up to, and pick a threshold that falls somewhere between the two that indicates to always use single threaded.

@oSumAtrIX
Copy link
Member

By default 1 << 16 classes are compiled for each dex file, reducing this number causes more dex files to be compiled but it does not consume this much memory anymore, but more dex files mean the app will load slower.

@oSumAtrIX
Copy link
Member

I am considering closing this issue as it doesn't really make sense from the standpoint of ReVanced Patcher to handle memory issues. It provides the option to select either single or multithreading, but what you make out of it, is not the concern of ReVanced Patcher anymore.

@oSumAtrIX oSumAtrIX added the Waiting on author Further information is requested label Feb 11, 2024
@Ushie
Copy link
Member Author

Ushie commented Feb 11, 2024

A client can't implement a solution that is better than a solution from the ReVanced Patcher itself, if ReVanced Patcher can figure out whether to use single or multithreading then the issue is solved universally on all clients, this prevents clients from having to restart the process when they encounter an OOM to run it in single threaded mode

@oSumAtrIX
Copy link
Member

It is not ReVanced Patcher's responsibility to care about the environment it runs on, even if it seems simpler to move the matter to ReVanced Patcher. ReVanced Patcher is what the name suggests. It is the single responsibility, and matters like the environment have to be handled by the environment. For that, it allows configuring it according to the environment's needs.

@Ushie
Copy link
Member Author

Ushie commented Feb 11, 2024

What about moving this to ReVanced Library? it fits the Library's motive and responsibility of unifying common code across clients

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature request Requesting a new feature that's not implemented yet Waiting on author Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants