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

Default to non-multithreaded until UI toggle support #1869

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

taylorkline
Copy link

Patching large applications (e.g. TikTok) on Android with multithreading simply doesn't work on multicore devices where the amount of memory given to the manager (regardless of the system RAM) is not sufficient.

It's more important that patching succeeds for all available use cases than for only some patching to succeed with faster performance. A UI option can be added later, but the multithreading option should be opt-out by default rather than opt-in.

Closes ReVanced/revanced-documentation#35
Closes #1454
Closes #1571
Closes #1595
Closes #1659
Closes #1661
Closes #1684
Closes #1759
Closes #1802
Closes #1817
Closes ReVanced/revanced-patches#2885
Closes #592
Closes ReVanced/revanced-patcher#193
Closes ReVanced/revanced-patches#1533
Closes ReVanced/revanced-patches#1608
Closes ReVanced/revanced-patches#1613
Closes ReVanced/revanced-patches#1630
Closes ReVanced/revanced-patches#190
Closes ReVanced/revanced-patches#2511
Closes ReVanced/revanced-patches#525

Patching large applications (e.g. TikTok) on Android with multithreading simply doesn't work on multicore devices where the amount of memory given to the manager (regardless of the system RAM) is not sufficient.

It's more important that patching succeeds for all available use cases than for only some patching to succeed with faster performance. A UI option can be added later, but the multithreading option should be opt-out by default rather than opt-in.

Closes ReVanced/revanced-documentation#35
Closes ReVanced#1454
Closes ReVanced#1571
Closes ReVanced#1595
Closes ReVanced#1659
Closes ReVanced#1661
Closes ReVanced#1684
Closes ReVanced#1759
Closes ReVanced#1802
Closes ReVanced#1817
Closes ReVanced/revanced-patches#2885
Closes ReVanced#592
Closes ReVanced/revanced-patcher#193
Closes ReVanced/revanced-patches#1533
Closes ReVanced/revanced-patches#1608
Closes ReVanced/revanced-patches#1613
Closes ReVanced/revanced-patches#1630
Closes ReVanced/revanced-patches#190
Closes ReVanced/revanced-patches#2511
Closes ReVanced/revanced-patches#525
@@ -279,7 +279,7 @@ class MainActivity : FlutterActivity() {
tmpDir,
Aapt.binary(applicationContext).absolutePath,
tmpDir.path,
true // TODO: Add option to disable this
false // TODO: Add option to toggle multithreading in UI
Copy link
Author

Choose a reason for hiding this comment

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

I'm happy to create an Issue on GitHub for this to keep track of this, if at all desired.

@taylorkline
Copy link
Author

@oSumAtrIX This should save you some headaches. I see you replying to all of the OOM issues opened on the assorted ReVanced GitHubs.

Tagging some recent committers for potential review: @Aunali321 @Ushie @TheAabedKhan @lamnhan066 @Domenic-MZS @Axelen123 @validcube

Thank you!

@oSumAtrIX
Copy link
Member

I think it's better to add a toggle if at all. Slowing down patching by the amount of cores worsens the experience for the majority of people whereas only fixes a problem for a handful of people. Single threading is still not a guarantee patching is gonna work though. The issue originates in how dex are compiled and that they consume lots of memory. From what I know this issue is a regression caused by a past migration from the smali library to Google's fork, whereas upstream had a fix involved for this particular problem.

@taylorkline
Copy link
Author

I'm of the opinion that maximum compatibility is preferable to speed.

...worsens the experience for the majority of people...

On my OnePlus 9 Pro, I'm timing about 60 seconds for a patch of YouTube, which is already long enough where I set my phone down and go on to do something else while I'm waiting.

Without multithreading, I'm getting 120 seconds. When I've already set my phone aside because it was already taking a minute, I don't know that it really makes a difference for it to take another minute.

...only fixes a problem for a handful of people.

Not sure I agree with that given the number of reports, both on here and on Reddit.

Single threading is still not a guarantee patching is gonna work though.

While true, the majority of reports on this issue are about TikTok, and TikTok patches fine without multithreading.

@Ushie
Copy link
Member

Ushie commented Apr 16, 2024

The complainers are loud (this is a saying), ReVanced Manager is used by hundreds of thousands or millions of people, during your tests this change brings a %100 decrease in patching speed which could be even worse for different devices, I believe the UI toggle is not much of a complicated feat, making it a bit unpreferable to merge this

@Domenic-MZS
Copy link
Contributor

Domenic-MZS commented Apr 17, 2024

Hello @taylorkline 🙌🏼 🌟 great initiative!

I agree with what @Ushie and @oSumAtrIX said. This idea seems to reflect the "survivorship bias". Only a few people of the failure cases are complaining that it does not work, compared to the majority of users. People who have a positive and functional experience aren't likely to be here opening issues or leaving good reviews.

Besides, it's a significant disadvantage to disable multithreading. In this case, it's preferable to have the guaranteed robustness and consistency of efficient and performant hex compilation with one or two non-working scenarios, rather than greatly slow the performance across all possible scenarios (guaranteed) to provide a may work may not (still hardware dependant) solution.

I think the main issue lies in the HEX compilation part; it could be refactored for better performance.

@Aunali321
Copy link
Member

I believe the better approach is how @Axelen123 has done it in compose manager (feat/compose-dev). It allows manager to execute patcher in another process and allows to change maximum alloted memory. It doesn't slow down patching rather speeds it up. I couldn't patch tiktok on my S21FE so i increased memory limit to 900mb (default is 512) and now i can patch it. It's better because it does not slow down patching process and because it allows the user the specify the max memory limit (in future maybe it can automated to use all available memory.)
SmartSelect_20240417_165004_ReVanced Manager

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment