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
base: main
Are you sure you want to change the base?
Conversation
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 |
There was a problem hiding this comment.
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.
@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! |
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. |
I'm of the opinion that maximum compatibility is preferable to speed.
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.
Not sure I agree with that given the number of reports, both on here and on Reddit.
While true, the majority of reports on this issue are about TikTok, and TikTok patches fine without multithreading. |
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 |
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. |
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.) |
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