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

Only import the translation used #451

Closed

Conversation

tadeubas
Copy link
Contributor

Description

Now it only imports the translation that is currently being used

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Copy link

codecov bot commented Aug 30, 2024

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 ⚠️
src/krux/translations_fr_FR.py 0.00% 1 Missing ⚠️
src/krux/translations_nl_NL.py 0.00% 1 Missing ⚠️
src/krux/translations_tr_TR.py 0.00% 1 Missing ⚠️
src/krux/translations_vi_VN.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tadeubas tadeubas marked this pull request as draft September 1, 2024 17:16
@tadeubas
Copy link
Contributor Author

tadeubas commented Sep 1, 2024

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 tadeubas marked this pull request as ready for review September 1, 2024 21:40
@tadeubas
Copy link
Contributor Author

tadeubas commented Sep 1, 2024

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

@odudex
Copy link
Member

odudex commented Sep 2, 2024

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
Copy link
Contributor Author

tadeubas commented Sep 3, 2024

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
Copy link
Member

odudex commented Sep 3, 2024

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
Copy link
Contributor Author

tadeubas commented Sep 4, 2024

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
Copy link
Member

odudex commented Sep 10, 2024

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

@tadeubas
Copy link
Contributor Author

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

  • Every new line of code will add something to the memory usage, even lines to delete or unimport things, if we want RAM so badly, we should start removing things instead of adding them, or we should abandon Python and start coding in C.
  • Otherwise, if we have enough RAM, isn't it ok to use it? What functionality do we need so much RAM for that every little KB would count? Isn't it just the PSBT signing process? (please correct me if I'm wrong, but I can't think of any other function that wouldn't be executed due to insufficient RAM).
  • So, we shouldn't aim to test RAM usage in every little piece of code, as that takes a lot of time and effort, but rather unimport things, especially when we need the most RAM, i.e. when the user signs a PSBT. That's when we should focus on analyzing the available RAM.

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
Copy link
Member

odudex commented Sep 10, 2024

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
Copy link
Contributor Author

tadeubas commented Sep 10, 2024

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

@tadeubas tadeubas closed this Sep 10, 2024
@tadeubas tadeubas deleted the efficient_memory_translations branch September 10, 2024 20:22
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