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

Keep enclosing class #100

Merged

Conversation

bacecek
Copy link
Collaborator

@bacecek bacecek commented Oct 18, 2023

Fixes #99

@bacecek bacecek requested a review from Jeffset October 18, 2023 14:51
@bacecek bacecek force-pushed the users/bacecek/keep_enclosing_class branch from ea47842 to 6d489b9 Compare October 18, 2023 20:14
@bacecek
Copy link
Collaborator Author

bacecek commented Oct 18, 2023

@Jeffset JFYI:
proguard-android-optimize.txt (default R8 config) already contains -keepattributes rule: https://cs.android.com/android-studio/platform/tools/base/+/mirror-goog-studio-main:build-system/gradle-core/src/main/resources/com/android/build/gradle/proguard-common.txt;l=1-8?q=proguard-common

Retrofit also contains it: https://github.com/square/retrofit/blob/master/retrofit/src/main/resources/META-INF/proguard/retrofit2.pro#L3.

So I personally don't think that declaring -keepattributes rule is something bad. Yatagan already contains -keep rules which have a greater impact on the size and startup time of the application in large projects than removing class attributes. And it's really hard to find a project where -keepattributes rule will not be delivered with one of external libraries or by AGP.

…g enclosingClass

enclosingClass can be null since R8 can remove class attributes
@bacecek bacecek force-pushed the users/bacecek/keep_enclosing_class branch from 6d489b9 to d287f07 Compare October 18, 2023 20:41
@Jeffset
Copy link
Contributor

Jeffset commented Oct 18, 2023

Thanks for the PR!

Yes, I was concerned about the idea of imposing global -keepattributes rules on the clients, as Yatagan needs this only in one place and that can even be worked around, as you did. Well, if these -keepattributes are very common and often other libraries have them, I guess it would be safe enough to have them in Yatagan too. But the workaround with the class name is still a bit better, imo, because it has no cost that I know of.

So let's go with the implemented approach for now. And if we find some downsides to it, we'll modify the minifier config as you initially suggested.

@Jeffset
Copy link
Contributor

Jeffset commented Oct 18, 2023

Yatagan already contains -keep rules which have a greater impact on the size and startup time of the application in large projects than removing class attributes.

Btw, how are the existing rules impact the size and startup time? If they are, that should be fixed.

# (1)
-keep class **.Yatagan$* {
    public * autoBuilder();
    public * builder();
}
# (2)
-keep @com.yandex.yatagan.Component interface *
-keep @com.yandex.yatagan.Component$Builder interface *

The rule (1) keeps just the two entry-point methods and the name of the class, right? Everything else inside the generated component implementation is still supposed to be eligible for optimization and mangling, isn't it?

And rules (2) just keep the component/builder interfaces. They have no executable code, as they are just interfaces, so no performance penalties should come of it.

Please, correct me if I'm wrong in interpreting the proguard rules meaning!

@bacecek
Copy link
Collaborator Author

bacecek commented Oct 19, 2023

Please, correct me if I'm wrong in interpreting the proguard rules meaning!

Sorry for my take, I was wrong about keep rule.

@Jeffset Jeffset merged commit 4c39ebd into yandex:main-1.4.1 Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants