revanced-manager icon indicating copy to clipboard operation
revanced-manager copied to clipboard

Default to non-multithreaded until UI toggle support

Open taylorkline opened this issue 2 months ago • 6 comments

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/revanced-manager#1454 Closes ReVanced/revanced-manager#1571 Closes ReVanced/revanced-manager#1595 Closes ReVanced/revanced-manager#1659 Closes ReVanced/revanced-manager#1661 Closes ReVanced/revanced-manager#1684 Closes ReVanced/revanced-manager#1759 Closes ReVanced/revanced-manager#1802 Closes ReVanced/revanced-manager#1817 Closes ReVanced/revanced-patches#2885 Closes ReVanced/revanced-manager#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

taylorkline avatar Apr 16 '24 20:04 taylorkline

@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!

taylorkline avatar Apr 16 '24 20:04 taylorkline

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.

oSumAtrIX avatar Apr 16 '24 20:04 oSumAtrIX

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.

taylorkline avatar Apr 16 '24 21:04 taylorkline

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

Ushie avatar Apr 16 '24 22:04 Ushie

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.

Domenic-MZS avatar Apr 17 '24 02:04 Domenic-MZS

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

Aunali321 avatar Apr 17 '24 11:04 Aunali321