-
Notifications
You must be signed in to change notification settings - Fork 37
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
Only import the translation used #451
Conversation
Codecov ReportAttention: Patch coverage is
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. |
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". |
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! |
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. |
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. |
Yes, using the singleton approach made |
Is this ready to merge? I wonder what could be done to increase patch coverage. |
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 |
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. |
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 |
Description
Now it only imports the translation that is currently being used
What is the purpose of this pull request?