-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
py/persistentcode: Store constants more efficiently #12008
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
base: master
Are you sure you want to change the base?
Conversation
Generated using: make -C mpy-cross clean && make -C mpy-cross cd tests/frozen/ ../../mpy-cross/build/mpy-cross frozentest.py For some reason, ordering of interned strings has changed, and the file still used the old "feature flags" in the header instead of the native arch and subversion combination. Signed-off-by: Armin Brauns <armin.brauns@embedded-solutions.at>
845946e
to
913ac6a
Compare
Some CI runs seem to get stuck, any ideas? |
This looks really interesting, thanks @arbrauns !
The infinite loop is the |
Ah! If you know where to look... That was supposed to be an |
@arbrauns I think you just need to change I'm curious though why this didn't come up in your local testing...? What compiler are you using? |
913ac6a
to
6120f4c
Compare
It seems like none of the files I tested against had a |
Code size report:
|
So this is +224 bytes on PYBV11 (for example). This is small, but not trivial. One consideration is that freezing undoes all of of the encoding and turns them into the exact structures that will be loaded. So the Python code frozen into the firmware gets no benefit. It also has no change on the RAM usage once the .mpy is loaded (because the loaded mp_float_t or integer value is the same). But, making .mpy files smaller is in general a good goal, so this is definitely worth spending some time thinking about! Is your use case dominated primarily by any one type (float, small int, or big int?) |
My use case is mostly floats, which are always exactly 4 bytes when binary-encoded, but potentially huge (10+ bytes) as strings. |
This allows users of mp_obj_int_to_bytes_impl() to allocate a buffer of the correct size. Signed-off-by: Armin Brauns <armin.brauns@embedded-solutions.at>
6120f4c
to
c4e9ac4
Compare
Previously, the bytes would always be interpreted as unsigned. Signed-off-by: Armin Brauns <armin.brauns@embedded-solutions.at>
eeda2e1
to
09505ad
Compare
Instead of being stored as strings: - Floats are stored as little-endian bytes - Integers are stored as a length byte and the minimum number of little-endian bytes required. This also necessitates a bump of the mpy version. Signed-off-by: Armin Brauns <armin.brauns@embedded-solutions.at>
The mpy version has changed. Signed-off-by: Armin Brauns <armin.brauns@embedded-solutions.at>
09505ad
to
578256a
Compare
Codecov Report
@@ Coverage Diff @@
## master #12008 +/- ##
==========================================
- Coverage 98.41% 98.38% -0.04%
==========================================
Files 155 155
Lines 20564 20600 +36
==========================================
+ Hits 20238 20267 +29
- Misses 326 333 +7
|
@arbrauns I was thinking about this some more... The current behavior seems inefficient, but because it's inherently variable-length, it's actually quite good for values that have a short decimal representation. i.e. For integers, the proposed change improves the efficiency of the current variable-length scheme. The current implementation is already variable-length and works well for small values. So I wondered maybe a small modification to the existing scheme would be worthwhile -- encode as BCD (i.e. nibble per digit, use 0xff as It's a bit tricky to implement this efficiently for encoding (because the current code gets a nice benefit from just using mp_obj_print_helper), but this is only enabled in mpy-cross so adding code size isn't so important. On the decode side it might not be too bad. |
That would be another good chunk of work that I don't currently have time to implement and test. As an alternative idea, how about supporting both float encodings in mpy (binary and via |
This is an automated heads-up that we've just merged a Pull Request See #13763 A search suggests this PR might apply the STATIC macro to some C code. If it Although this is an automated message, feel free to @-reply to me directly if |
Instead of being stored as strings:
This yields .mpy file space savings of 0.8% to 17% for the scripts I tested with.