perf(Bring back twitter): Optimize string replacement

[kitadai31]

https://t.me/pikopatches/1/17451

The current implementation has poor performance as it iterates over all string elements for each entry in the stringsMap.
DOM operations using Java XML API are expensive, but on the other hand, Map lookups are fast.
So, the string replacement should be done in a single iteration.

I tested performance on a very low-end phone (Snapdragon 410, 2GB RAM) with ReVanced Manager.

Before PR:

values: 2373 ms
values-en-rGB: 1886 ms
values-ru: 1983 ms
values-hi: 1986 ms
values-pt-rBR: 17 ms
values-tr: 1946 ms
values-zh-rCN: 1950 ms
values-zh-rTW: 2008 ms
Total: 14149 ms

After PR:

values: 631 ms
values-en-rGB: 339 ms
values-ru: 445 ms
values-hi: 453 ms
values-pt-rBR: 7 ms
values-tr: 367 ms
values-zh-rCN: 400 ms
values-zh-rTW: 392 ms
Total: 3034 ms

[ADDED]
Furthermore, with this change, adding more strings to the map will have little impact on the performance of the patch.
Thus, more "X" can be replaced with "Twiter".

---

I have a few questions before this PR is ready.

• Do you really need to add strings that aren't present in the original strings.xml?

If a string does not exist in the original strings.xml, it was probably deleted from the app.
If so, I think adding it is no point because it will never displayed in the app.
In addition I can remove mutableStringsMap.

• Do you really need to create a new string.xml when it doesn't exist?

In my opinion, it is not needed to create a new strings.xml even if a specific language string.xml is not found,
When users merged the split APKs, they may have removed unnecessary language splits, in which case this patch does not need to add strings.xml for languages ​​that do not exist.
Also, if a user unintentionally excludes the language split for their language, after applying this patch they will experience a strange situation where only the 60 strings included in this patch are translated.

At the time this patch was created, full APK of X was provided.
However, now that users have to merge the split APKs themselves, I believe that this behavior needs to be reconsidered.

• What is // log which keys were not found or failed ?

I can't get what this comment means.
I think logging the missing strings is good idea instead of adding them silently.

[crimera]

What is // log which keys were not found or failed ?

I think this is for testing that we forgot to remove.

Do you really need to create a new string.xml when it doesn't exist?

Do you really need to add strings that aren't present in the original strings.xml?

Fair points. Now that I look at the code, they really are redundant. One counter point. It may be that the original apk does not provide translations for the user's native language, so they create a custom translation for it. But seeing that no one actually did that, then I think it is safe to remove them after all.

[kitadai31]

Should I add logging code for strings which was not found?

[IndusAryan]

yeah, new implementation is a lot better

[kitadai31]

Hey, after I submitted this PR, I found a old code in the git log that replaces all "X" with "Twitter".
And I was curious why it was slow, so I looked into it.
https://github.com/crimera/piko/blob/2c63ff1bc1444ed6d83f911becca4289001c306c/src/main/kotlin/crimera/patches/twitter/misc/bringbacktwitter/BringBackTwitterResourcePatch.kt

In conclusion, it was a problem with using setTextContent().

This part was causing the performance issue.

for (i in 0 until strings.length) {
    val string = strings.item(i) as Element
    if (!string.getAttribute("name").contains("api_key")) {
        string.textContent = string.textContent.replace("X", "Twitter")
    }
}

This code calls setTextContent() for each string. (= 6000-7000 times for each language.)
Kotlin makes setters like setTextContent() look like a simple assignment, but in reality setTextContent() is a very slow operation.
I tried rewriting this part like this:

for (i in 0 until strings.length) {
    val string = strings.item(i) as Element
    if (!string.getAttribute("name").contains("api_key")) {
        val text = string.textContent
        if (text.contains("X")) {
            string.textContent = text.replace("X", "Twitter")
        }
    }
}

This improved performance incredibly.

Old code:
res/values-de/strings.xml: 1325 ms
res/values-ko/strings.xml: 1272 ms
res/values-pt/strings.xml: 1220 ms
res/values-ro/strings.xml: 1256 ms
res/values-zh-rCN/strings.xml: 1231 ms
res/values-ar-rEH/strings.xml: 985 ms
res/values-hu/strings.xml: 1506 ms
res/values-fr/strings.xml: 1266 ms
res/values-ja/strings.xml: 1235 ms
res/values-uk/strings.xml: 1264 ms
res/values-it/strings.xml: 1236 ms
res/values-in/strings.xml: 1269 ms
res/values-pl/strings.xml: 1321 ms
res/values-hi/strings.xml: 1289 ms
res/values-ru/strings.xml: 1230 ms
res/values-ms/strings.xml: 1247 ms
res/values-th/strings.xml: 1214 ms
res/values-nl/strings.xml: 1293 ms
res/values-en-rGB/strings.xml: 1632 ms
res/values-zh-rTW/strings.xml: 1287 ms
res/values-zh-rHK/strings.xml: 1241 ms
res/values-es/strings.xml: 1297 ms
res/values-fi/strings.xml: 1250 ms
res/values-vi/strings.xml: 1299 ms
res/values-ar/strings.xml: 1303 ms
res/values-sv/strings.xml: 1257 ms
res/values-tr/strings.xml: 1290 ms
res/values/strings.xml: 2050 ms

↓↓↓↓↓↓

New code:
res/values-de/strings.xml: 128 ms
res/values-ko/strings.xml: 95 ms
res/values-pt/strings.xml: 70 ms
res/values-ro/strings.xml: 75 ms
res/values-zh-rCN/strings.xml: 74 ms
res/values-ar-rEH/strings.xml: 68 ms
res/values-hu/strings.xml: 70 ms
res/values-fr/strings.xml: 68 ms
res/values-ja/strings.xml: 57 ms
res/values-uk/strings.xml: 29 ms
res/values-it/strings.xml: 48 ms
res/values-in/strings.xml: 50 ms
res/values-pl/strings.xml: 48 ms
res/values-hi/strings.xml: 70 ms
res/values-ru/strings.xml: 61 ms
res/values-ms/strings.xml: 49 ms
res/values-th/strings.xml: 58 ms
res/values-nl/strings.xml: 51 ms
res/values-en-rGB/strings.xml: 50 ms
res/values-zh-rTW/strings.xml: 55 ms
res/values-zh-rHK/strings.xml: 57 ms
res/values-es/strings.xml: 51 ms
res/values-fi/strings.xml: 51 ms
res/values-vi/strings.xml: 99 ms
res/values-ar/strings.xml: 55 ms
res/values-sv/strings.xml: 51 ms
res/values-tr/strings.xml: 51 ms
res/values/strings.xml: 64 ms

Please note that this result is measured on a Windows PC with Ryzen 5 5500U + 8GB RAM using ReVanced CLI.
So this times cannot be comparable with the time above.
I have not tried the old code on the low-end phone, but it is not hard to imagine that the results would be very bad.

This execution time is almost the same as the current process using replacement map.
With this change, performance is no longer an issue, so in the future we could consider simply replacing all X with Twitter.

But this is beyond the scope of this PR.
I just tell this for your information.

[kitadai31]

I pushed the change.
I left a log code that shows the number of strings which was not found.
Please check if this is fine.

However, a new problem with pt-rBR languages arose.
This patch has a pt-rBR strings map, but the X APK only has values-pt and doesn't have values-pt-rBR.
pt-rBR strings will not be applied after this change.

I don't know the difference between normal pt and pt-rBR, but if the strings in the pt_rBR map is taken directly from Twitter 9.98.0 apk, I think I can change values-pt-rBR to values-pt directly.

I'll look into it tomorrow or the day after tomorrow.

[crimera]

pt-rBR languages

It's for Portuguese Region Brazil. It seems I missed this one. We actually need to create the strings for that.

[kitadai31]

After researched pt_rBR strings in this patch, I found that the grammar and wording was slightly different from the normal Portuguese (values-pt) contained in the original X APK, and that it was indeed Brazilian Portuguese.

One counter point. It may be that the original apk does not provide translations for the user's native language, so they create a custom translation for it.

This was real...

However, for other languages ​​it is still preferable not to add strings.xml if it does not exist.
Therefore, I'm moving the pt_rBR to customstringsupdater and adding custom logic for Brazilian Portuguese like Japanese.
It creates pt-rBR strings.xml and adds strings, instead of replacing existing strings.xml.

@r7kings Hello, if I'm correct, you are the one who created the pt_rBR strings in this patch.
Is my plan correct?
Is the pt_rBR string intended to be added to the newly created values-pt-rBR directory, and not to the existing values-pt directory, right?

---

After that, I will resolve conflicts.
But I can see some debug codes (?) in c20e16f.
measureExecutionTime and println("parsing: ${stringsFile.parent}")

@crimera Is this intended? Should I keep these code after merging conflicts?

[crimera]

Should I keep these code after merging conflicts?

Nah, I forgot to remove it.

Oops, accidentally closed the issue. My bad, I just woke up and misclicked

[kitadai31]

Sorry for the late reply.
I pushed the changes, and this PR is now ready to be reviewed or merged.

By the way, I removed the missing strings logs at 3448546.

It turned out that when user removed some languages from split apk and SettingsPatch was applied prior to this patch, this log always show warnings because SettingsPatch adds other languages files.
For example, when a user chose only English split when merging split APKs, this log show a warning for each en-rGB, ru, hi, tr, zh-rCN, and zh-rTW languages that says "60 keys were not found in ..."

Therefore, I removed the log.

(If you still think the log is needed, please let me know.)

[kitadai31]

I forgot to tell you one important thing.
Sincerely, please merge with squash and merge.

When I resolved the conflict on my local repository, I should have rebased the working branch on the dev branch instead of merging the dev branch. 😅
(Many "Merge branch 'refs/heads/dev' into fix/bring-back-twitter-performance")
As a result, this commit log is dirty, so doing a normal merge will mess up the commit log. 😅