Add getters to ResConfig, compute qualifiers lazily and swap out equals and hashCode implementations

[X1nto]

Parsing out possible configurations outside of ResConfig's class currently requires calling toQualifiers() and then manually filtering out configurations based on their string values. This approach becomes problematic when dealing with languages, because simply getting the language of a ResConfig isn't possible without reflection unless we iterate over all possible languages. So, this adds getters.

The second thing this PR addresses is how qualifiers are used. Previously, qualifiers were computed whether or not they were needed by APKTool. As far as I'm aware, the only need of built qualifiers outside of hashCode() and equals() (more on this later) is writing resources to files. Since some programs using apktool-lib (including our internal tools at APKMirror) may not need to extract these resources to files, we might as well avoid extra string allocations unless explicitly needed by the library. As for equals() and hashCode(), I think using qualifiers were a wrong choice for this because qualifiers don't include all fields, such as mMinorVersion. While I doubt that 2 same resource configs with only minor versions differing will ever exist in the same APK, I still think it's better to cover all fields of the class. Plus, with qualifiers being lazily-calculated in this new approach, it doesn't make much sense to use them in these methods.

I initially wanted to make this PR with the sole purpose of adding getters, however, seeing the opportunity, I took care of the other stuff. I may be wrong with my additions, so please, let me know if there's a reason why qualifiers should be used, or if you just prefer to make these commits into their own separate PRs.

[IgorEisberg]

This isn't necessary for the official Apktool. No one else has complained about it, and it just adds unnecessary complexity.
You can keep a private fork for your own needs, just like I do.
Why complicate the official repo with something others will have to deal with later?

[X1nto]

This isn't necessary for the official Apktool. No one else has complained about it, and it just adds unnecessary complexity. You can keep a private fork for your own needs, just like I do. Why complicate the official repo with something others will have to deal with later?

apktool-lib is published as its own library on maven, that means it's intended to be used by others, right? Otherwise it wouldn't be published and APKTool would work just fine. Plus, what's wrong with contributing to something that others might be using? Also, why do you think the repo is getting complicated? If your concern is about the changes to hashCode and equals, I already said that I can either push those changes in a separate PR or revert them completely and keep this PR open just for the getters.

[IgorEisberg]

apktool-lib is published as its own library on maven, that means it's intended to be used by others, right?

Yes, the library is pushed to be used as-is. Otherwise, the source code is available to fork.

Plus, what's wrong with contributing to something that others might be using? Also, why do you think the repo is getting complicated? If your concern is about the changes to hashCode and equals, I already said that I can either push those changes in a separate PR or revert them completely and keep this PR open just for the getters.

I have no problem with the getters, but the code duplication is ridiculous for no tangible benefit, just complexity. Pre-generating a qualifiers string once per config is really not a big deal.
Also, there's a reason we use the qualifiers for equality and hashing. Fields that aren't used in qualifiers should be completely ignored for both. If they were significant, they would inevitably have been part of the qualifiers, meaning the qualifiers are the arbiters of equality, not the individual fields.
The concept of "invalid config" is directly tied to the inability to generate 100% accurate qualifiers.

In any case, this isn't my decision, I'm just giving my two cents.

[iBotPeaches]

I think its valid that we ship apktool-lib, it could act more library friendly. Looking at how its operated in past we basically generated qualifiers on the fly because thats all we needed them for.

My concern is the large duplication of the qualifier checks as we previously relied on the writing of the qualifier to know if it was invalid or not. That computation split out now has to do some heavy lifting in semi-duplication of each qualifier check.

The getters don't bother me as much, but maybe if the Java 14 records compile down to regular ole getters that might clean up our logic to be less verbose. I recently had to move to Java11 for Maven publishing, but still can produce a Java8 valid binary. I don't think its out of the realm to say we could move to Java14 for building if it still produces a Java8 compatible binary.

[IgorEisberg]

Yes, I have no problem with adding the getters even if apktool itself doesn't use them.
The rest of the changes are unreasonable. There's a direct logic chain that assumes: 1 unique config = 1 unique qualifiers string = 1 unique resource dir in output.
We pre-generate the qualifiers string and decide whether it is valid or not as a secondary output, and we call isInvalid immediately after we create the config in BinaryResourceParser.
The design of ResConfig was made to be 100% immutable, and a final qualifiers string field is part of that design.

[X1nto]

I think its valid that we ship apktool-lib, it could act more library friendly. Looking at how its operated in past we basically generated qualifiers on the fly because thats all we needed them for.

My concern is the large duplication of the qualifier checks as we previously relied on the writing of the qualifier to know if it was invalid or not. That computation split out now has to do some heavy lifting in semi-duplication of each qualifier check.

I understand your concern, but I genuinely couldn't think of a way to make isValid and qualifiers work with this lazy-compute approach. If you have a better suggestion, or if you simply prefer to keep old behavior and precompute this qualifier string, that's fine by me.

The rest of the changes are unreasonable.

I made these changes to reduce the memory footprint taken by each ResConfig, as every string is an extra allocation. Again, if you're not comfortable with this, I can revert this change, I just want to understand the reason behind doing this the way you did it, since you guys have way more experience than me in this codebase.

The getters don't bother me as much, but maybe if the Java 14 records compile down to regular ole getters that might clean up our logic to be less verbose. I recently had to move to Java11 for Maven publishing, but still can produce a Java8 valid binary. I don't think its out of the realm to say we could move to Java14 for building if it still produces a Java8 compatible binary.

While records could help with this, I don't think you can produce a Java 8 compatible library since records rely on java.lang.Record, which isn't available on older runtimes. More realistic alternatives to this are either Lombok or Kotlin, but both of them are huge dependencies that are out of the scope of this PR.

[X1nto]

Looks like you already committed getters to the main branch. I would've preferred if you waited for my reply so we could've sort this out together, instead of cherry-picking my changes. No offense to anyone, as I have no say in what and how goes to your repo, but I think it doesn't take much effort to at least give me a heads up.

[iBotPeaches]

I understand your concern, but I genuinely couldn't think of a way to make isValid and qualifiers work with this lazy-compute approach. If you have a better suggestion, or if you simply prefer to keep old behavior and precompute this qualifier string, that's fine by me.

Do you have more details around your usecase? Trying to visualize what you are trying to do in your userland.

I made these changes to reduce the memory footprint taken by each ResConfig, as every string is an extra allocation. Again, if you're not comfortable with this, I can revert this change, I just want to understand the reason behind doing this the way you did it, since you guys have way more experience than me in this codebase.

I'll await Igor's comments on this one. I don't have much comfort in speaking confidently in the allocation/perf area.

While records could help with this, I don't think you can produce a Java 8 compatible library since records rely on java.lang.Record, which isn't available on older runtimes. More realistic alternatives to this are either Lombok or Kotlin, but both of them are huge dependencies that are out of the scope of this PR.

Yeah I guess both of those deps look bad. We ended up merging basic getters manually generated.

Looks like you already committed getters to the main branch. I would've preferred if you waited for my reply so we could've sort this out together, instead of cherry-picking my changes. No offense to anyone, as I have no say in what and how goes to your repo, but I think it doesn't take much effort to at least give me a heads up.

Yep looking back we should have added your "co-authored by" - sorry about that.

[X1nto]

Do you have more details around your usecase? Trying to visualize what you are trying to do in your userland.
I'll await Igor's comments on this one. I don't have much comfort in speaking confidently in the allocation/perf area.

One of our tools use apktool-lib to simply extract resources table into the memory and grab necessary resources from the APK on-the-go. We don't write the resources to files, and from what I've seen via toQualifiers() usages, the only time apktool-lib ever uses toQualifiers() (apart from equality checks) is when writing resources to disk to generate correct folders. From my perspective, qualifiers take up extra memory for this program because it never writes resources to disk. Unfortunately, in order to lazily compute these qualifiers, I had to duplicate logic for isInvalid, because otherwise isInvalid would become lazily-computed too (maybe you're okay with that? To be fair I don't think this would break anything). From what I've tested, the memory footprint is lower and the changes to equals and hashCode makes no difference in CPU time. I could be wrong here, as this is both machine and JVM-dependent.

Yep looking back we should have added your "co-authored by" - sorry about that.

It's alright! My comment wasn't really about being included as a commit author :D

[IgorEisberg]

The PR wasn't a replacement to your PR, nor a "cherry-picking". I simply exposed the fields, standard POJO style. Your PR is still being discussed.
I still don't understand why you got so fixated on a handful of precomputed qualifier strings while Apktool does many other allocations that aren't necessarily being used in all cases (like proactively decoding strings in string pools during parsing, each is a new string allocation). Trying to micro-optimize every bit of Java code is an endless cycle.

[X1nto]

I still don't understand why you got so fixated on a handful of precomputed qualifier strings while Apktool does many other allocations that aren't necessarily being used in all cases (like proactively decoding strings in string pools during parsing, each is a new string allocation).

I'm aware of how string decoding works in APKTool, I didn't have enough time to properly benchmark and fix that. I also didn't get "fixated" on anything, I simply wanted to suggest a more-efficient approach for handling ResConfigs. The fact that I made this PR right now doesn't mean that I'm not looking forward to suggest other improvements.

[IgorEisberg]

I see. Alright then, looking forward to it.