selfcustody / krux

Open-source signing device firmware for Bitcoin
https://selfcustody.github.io/krux/
Other
185 stars 37 forks source link

Only import the translation used #451

Closed tadeubas closed 2 months ago

tadeubas commented 2 months ago

Description

Now it only imports the translation that is currently being used

What is the purpose of this pull request?

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 82.14286% with 5 lines in your changes missing coverage. Please review.

Project coverage is 94.15%. Comparing base (1b03495) to head (3d316d5). Report is 73 commits behind head on develop.

Files with missing lines Patch % Lines
src/krux/translations_de_DE.py 0.00% 1 Missing :warning:
src/krux/translations_fr_FR.py 0.00% 1 Missing :warning:
src/krux/translations_nl_NL.py 0.00% 1 Missing :warning:
src/krux/translations_tr_TR.py 0.00% 1 Missing :warning:
src/krux/translations_vi_VN.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #451 +/- ## =========================================== - Coverage 94.21% 94.15% -0.06% =========================================== Files 59 67 +8 Lines 7259 7273 +14 =========================================== + Hits 6839 6848 +9 - Misses 420 425 +5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tadeubas commented 2 months ago

Given our developers' recent discoveries about how memory is managed, I need to switch to importing into different modules (files) instead of using the same module (file) to truly achieve "memory_efficient_translations".

tadeubas commented 2 months ago

Hey @odudex , can you see if there has been any memory improvement on your part now?

odudex commented 2 months ago

Hey @odudex , can you see if there has been any memory improvement on your part now?

I do see! 33KB!? Awesome! This is the way! Congratulations!

tadeubas commented 2 months ago

Hey @odudex I think this now improves the memory a little at startup, when accessing tools and other minor gains regarding Settings configs across all the code.

odudex commented 2 months ago

From my initial test, which did not load Tools, I observed a slight decrease in the RAM use reduction difference from the develop branch; it was approximately -33KB and is now around -32KB. However, if you have confirmed that this method of full path import and deletion is more efficient for unloading modules, it could likely be applied in many other areas.

tadeubas commented 2 months ago

Yes, using the singleton approach made Settings() persistent, so it wouldn't be disposed by gc.collect(). Every time we use Settings() it creates new objects, but they are removed as soon as they are unused and gc.collect() is called. So I've removed my change to gain ~1KB in free memory

odudex commented 2 months ago

Is this ready to merge? I wonder what could be done to increase patch coverage.

tadeubas commented 2 months ago

I did some extensive testing and came to the following conclusions:

So even if this PR frees up a few KB, I think it's useless, this can be merged, but we'll need another PR to do the real work, i.e. unimport everything before signing a transaction and import everything we need right after the process to show things on the screen to the user. The best way to achieve this is by modifying boot.py to delete the Home object and unimport stuff and keep only the wallet and PSBT/embit related stuff in memory to do the signing process, then instantiate the Home object again and go straight to the screen after signing... some modifications will need to be done in Home as well, but I think this is the best scenario for RAM and efficiency memory usage.

odudex commented 2 months ago

Isn't it just the PSBT signing process?

PSBT signing is not "just" a thing, it is the main purpose of the project.

I disagree with you. I think your PR is valuable; it's practical and functional. The only thing worrying me is your hesitation. Yes, each line of code incurs a "cost" in ROM, and some in RAM, but these costs are the price we pay for features. In a refactor, we typically don't acquire new features, so in my opinion, it's less justifiable to undertake a refactor that reduces RAM efficiency, which is not the case here. To streamline reasoning, enhance productivity, and maintain objectivity, I suggest narrowing the focus of our discussions to the pull request, and maybe leave more theoretical, structural and philosophical ideas to "Discussions". We don't need to rush and do any extreme measure; we just need to walk in the right direction.

tadeubas commented 2 months ago

Exactly, this PR does not substantially contribute to the main purpose of the project, it just shifts the focus away from the correct place where we should be concerned about freeing up RAM... I will close this to focus on the isolation of the signing process, not small KB improvements in RAM