revanced-integrations
revanced-integrations copied to clipboard
ReVanced
feat: remove usage of reflection in integrations
🐞 Issue
Some patches (for example custom-playback-speed and default-video-quality use reflection inside the integrations. That should be changed as this decreases the user experience because reflection makes the youtube app slower.
❗ Solution
Remove the usage of reflection and move that logic to the actual Patch.
❓ Motivation
Follow the general unspoken rule of java development to not use reflection at all. Moving logic that currently uses reflection will move the time consuming logic from runtime to compile time which will increase the user experience.
because reflection makes the youtube app significantly slower
I can not reproduce this.
because reflection makes the youtube app significantly slower
I can not reproduce this.
Get an older phone and try again. You have a OnePlus 8 Pro, of course you cannot.
No need to ask anyone else, I just have to compare it on my device. I have also tried it on an old tablet where I can not reproduce it.
No need to ask anyone else, I just have to compare it on my device. I have also tried it on an old tablet where I can not reproduce it.
How did you try it?
Also maybe its not that big of an effect right now. But when more reflection continues to be used, it will obv increase. Directly access fields and methods is always faster than accessing and calling them using reflection. Thats the point
How did you try it? Also maybe its not that big of an effect right now
Yes & Yes.
So general rule of any development is to make the code run the fastest way you can possibly do. Thats why the question about if reflection should be used or not is unneccessary. Theres only one answer for that.
So the general rule of any development is to make the code run the fastest way you can possibly do
No, this is the wrong approach. Readability and understandable code are way better than a complicated approach, especially in such cases and where the difference is unnoticable as prevs said.
So general rule of any development is to make the code run the fastest way you can possibly do.
Since we're able to achieve this goal and still be able to be failsafe using fingerprints, it should be changed to patches.
So the general rule of any development is to make the code run the fastest way you can possibly do
No, this is the wrong approach. Readability and understandable code are way better than a complicated approach, especially in such cases and where the difference is unnoticable as prevs said.
changing the integrations to not use reflection does not make the code less readable or less understandable. Also, as previously said, it might no be recognizable for you for now. But if the use of reflection increases it will be. I've already read comments of people complaining that revanced is slower than vanced or stock youtube was.
changing the integrations to not use reflection does not make the code less readable or less understandable
It very much does, due to now having to resort to an abstract approach where the patch has to embed the code - which was readable in the integrations - inside the app.
But if the use of reflection increases it will be.
It has to increase significantly. Reflective calls are being made thousands of times in a short time.
I've already read comments of people complaining that revanced is slower than vanced or stock youtube was.
Placebo as there is no significant difference yet.
It very much does, due to now having to resort to an abstract approach where the patch has to embed the code - which was readable in the integrations - inside the app.
I don't see how that will decrease readability. App has to be decompiled to completely understand what the patches do anyways.
It has to increase significantly. Reflective calls are being made thousands of times in a short time.
Which is why it should be avoided to use for us. Especially using reflection to looping through all methods in a class.
Placebo as there is no significant difference yet.
Not for you, but you can't speak for the general user.
Also I thought that this discussion was over? You already agreed on changing the patches later.
Also read this: https://ncrcoe.gitbooks.io/java-for-small-teams/content/specifics/1000_do_not_use_reflection.html
I don't see how that will decrease readability.
As I said previously:
due to now having to resort to an abstract approach where the patch has to embed the code - which was readable in the integrations - inside the app.
Which is why it should be avoided to use for us.
Once again:
It has to increase significantly.
Hence it is not a high priority.
but you can't speak for the general user
I am the general user as I have tried what the general user has tried and was not possible to reproduce. Unless no reproducible steps or something like a recording is given you can not assume that it is not a placebo.
Also read this
No need to. I already said:
Readability and understandable code are way better than a complicated approach, especially in such cases and where the difference is unnoticable as prevs said.
Also:
Also I thought that this discussion was over?
Reason being because you continued it.
Why are you continuing to argue if you say the discussion is over?
I did not continue it? You did by commenting this issue? You asked to create an issue which I did.
https://github.com/revanced/revanced-integrations/issues/96#issuecomment-1192738420:
I can not reproduce this.
I can not reproduce this.
Which started the discussion again.
I can not reproduce this.
Which started the discussion again.
Please stop going off topic. The comment was related to this topic.
I think that if you really need such a change to be made ASAP and the maintainers are currently not in favor of it, then you should fork the patch and maintain your own flavor of Revanced Integrations @TheJeterLP
The maintainers have made it so extensible, that anyone can take the project and extends or modify, I believe so, but I may be wrong
You will never force anyone to make a change by commenting here, even if you are right or wrong, this doesn't matter here. If you're perfectly sure that your change should make it upstream, then prove that your Revanced Integrations and your fork can out stand the official Revanced Integrations for older phone users, once you made that clear, maybe things will move upstream.
I think that if you really need such a change to be made ASAP and the maintainers are currently not in favor of it, then you should fork the patch and maintain your own flavor of Revanced Integrations @TheJeterLP
The maintainers have made it so extensible, that anyone can take the project and extends or modify, I believe so, but I may be wrong
You will never force anyone to make a change by commenting here, even if you are right or wrong, this doesn't matter here. If you're perfectly sure that your change should make it upstream, then prove that your Revanced Integrations and your fork can out stand the official Revanced Integrations for older phone users, once you made that clear, maybe things will move upstream.
I was a team member when writing that issue and we discussed about it.
Is this issue still relevant? It's now a year later, and custom-video-speed no longer uses reflections.
remember-video-quality use reflections only once when a new video first loads, and not anytime else.
The only other examples I can think of that still uses reflections is Sponsorblock. The seekbar drawing code of Sponsorblock (the set seekbar frame rectangle code) would be the biggest candidate, as it's called when the frame rectangle is updated (switching to/from full screen, or when closing/opening the app). I wondered how slow the reflections usage was for this one example, so I added some performance logging around the method, and to my surprise the runtime showed up as less than 100 nanosecond. That's less than 1/10th of one millionth of 1 second. If this ran in a loop this might be an issue, but the frame rect reflections call is a single call. So the performance cost of using reflections there is a meaningless amount of time.
The sponsorblock seek code also uses reflections, but that is called only when skipping segments, which is a maximum of maybe a dozen times for each videos (also a meaningless performance overhead).
Is there anywhere else reflections is still used? If there is nowhere else, then this issue can probably be closed.
Reflection has nearly no impact on performance whatsoever, but it's not really good design here and should ideally never be used. Closing though until it opposes an actual problem ala https://en.m.wikipedia.org/wiki/Ostrich_algorithm