py/persistentcode: Store constants more efficiently - #12008

Some CI runs seem to get stuck, any ideas?

This looks really interesting, thanks @arbrauns !

Some CI runs seem to get stuck, any ideas?

mpy-cross is getting stuck in an infinite loop while compiling extmod/asyncio/__init__.py

The infinite loop is the for (mp_int_t mask = (mp_int_t)0xff << 8 * (sizeof(mp_int_t) - 1); mask != 0; mask >>= 8) { in save_code (persistentcode.c:656). Seems to be when val==0.

Ah! If you know where to look... That was supposed to be an mp_uint_t; mp_int_t uses arithmetic right shifts and can never reach zero, of course.

@arbrauns I think you just need to change for (mp_int_t mask to for (mp_uint_t mask.

I'm curious though why this didn't come up in your local testing...? What compiler are you using?

I'm curious though why this didn't come up in your local testing...? What compiler are you using?

It seems like none of the files I tested against had a 0 integer constant.

Code size report:

   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:  +432 +0.054% standard
      stm32:  +272 +0.069% PYBV10
     mimxrt:  +264 +0.074% TEENSY40
        rp2:  +280 +0.086% PICO
       samd:  +200 +0.077% ADAFRUIT_ITSYBITSY_M4_EXPRESS

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.

Codecov Report

❌ Patch coverage is 87.50000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.38%. Comparing base (14c2b64) to head (578256a).
⚠️ Report is 4096 commits behind head on master.

@@            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     

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

@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. 1.3 is 3 bytes, but a value with many decimal places is potentially e.g. 15+ bytes.
With the proposed change, because mpy-cross always runs with MP_FLOAT_IMPL=double we will now always emit 8 bytes for a floating point value. So we bound the worst case, but make the best (and I suspect average) case worse.

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 .). For doubles with 15-16 digits, you'll now be at ~8 bytes worst case. Although you do still need the length byte.

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 mp_obj_print_helper), and encoding each float constant as whichever variant is shorter? It would strictly make float decoding more complex than it was before, of course, but as far as I can tell, the binary float decoding should be fairly trivial and not have much code size impact.

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.