Skip to content

py/gc: Support multiple heaps (version 3). #8526

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

Closed
wants to merge 3 commits into from

Conversation

rknegjens
Copy link
Contributor

@rknegjens rknegjens commented Apr 12, 2022

Update of PR 3580 from @aykevl that enables addition of heap space at runtime. See the original PR for background and motivation.

This PR rebases the original onto latest master, integrating updates to the GC since then (Jan 2018). Unix port is passing tests with and without multiple heaps enabled.

It also adds some C macro optimisations to reduce firmware image sizes when multiple heaps are disabled.

Image size diffs versus current master (28e7e15) when multi-heap disabled:

       bare-arm:    +0 +0.000%
    minimal x86:    +0 +0.000%
       unix x64:   -16 -0.003%
    unix nanbox:   -20 -0.004%
          stm32:    -8 -0.002% PYBV10
         cc3200:    +0 +0.000%
        esp8266:    +8 +0.001% GENERIC
          esp32:    +0 +0.000% GENERIC
            nrf:   -20 -0.011% pca10040
            rp2:    +0 +0.000% PICO
           samd:    -4 -0.003% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Note that only the esp8266 image gets slightly bigger (+8).

Image size diffs versus current master when multi-heap is enabled with
MICROPY_GC_MULTIHEAP=1 (but not extra code to add more heaps):

    unix x64: +1032 +0.197% [incl +544(bss)]
       esp32:  +592 +0.039% GENERIC[incl +16(data) +264(bss)]

@rknegjens rknegjens force-pushed the multiheap-2-rebase branch 2 times, most recently from eb1cf5f to 2d5128a Compare April 12, 2022 23:19
@dpgeorge dpgeorge added the py-core Relates to py/ directory in source label Apr 12, 2022
@dpgeorge
Copy link
Member

CI sometimes has spurious failures (eg on uasyncio_gather), but in this case the "unix port /qemu_arm" failure is real (and should be easy to fix).

@micropython micropython deleted a comment from acidblock Apr 13, 2022
@rknegjens rknegjens force-pushed the multiheap-2-rebase branch from 2d5128a to f1d9156 Compare April 13, 2022 02:17
@rknegjens
Copy link
Contributor Author

rknegjens commented Apr 13, 2022

CI sometimes has spurious failures (eg on uasyncio_gather), but in this case the "unix port /qemu_arm" failure is real (and should be easy to fix).

Yea there were a few unix port variants failing that had been passing, which was due to a quick refactor. Issue is fixed now.

The only variant failing now is macos, due to singlee test: repl_inspect. Any idea what could be causing that?

@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2022

Codecov Report

Merging #8526 (f2198b4) into master (66b5c4c) will decrease coverage by 0.04%.
The diff coverage is 93.17%.

@@            Coverage Diff             @@
##           master    #8526      +/-   ##
==========================================
- Coverage   98.25%   98.20%   -0.05%     
==========================================
  Files         154      154              
  Lines       20288    20337      +49     
==========================================
+ Hits        19933    19971      +38     
- Misses        355      366      +11     
Impacted Files Coverage Δ
shared/timeutils/timeutils.h 100.00% <ø> (ø)
py/gc.c 96.23% <93.17%> (-2.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d7c168...f2198b4. Read the comment docs.

@dpgeorge
Copy link
Member

I haven't seen repl_inspect fail before (at least randomly fail). The error diff from the CI is:

--- /Users/runner/work/micropython/micropython/tests/results/cmdline_repl_inspect.py.exp	2022-04-13 02:23:06.000000000 +0000
+++ /Users/runner/work/micropython/micropython/tests/results/cmdline_repl_inspect.py.out	2022-04-13 02:23:06.000000000 +0000
@@ -1,6 +1,3 @@
 test
-MicroPython \.\+ version
-Use \.\+
->>> # cmdline: -i -c print("test")
->>> # -c option combined with -i option results in REPL
->>> 
+# cmdline: -i -c print("test")
+# -c option combined with -i option results in REPL

I don't know... maybe push again to re-trigger CI and see if it happens again.

@rknegjens rknegjens changed the title WIP: py/gc: Support multiple heaps (version 3). py/gc: Support multiple heaps (version 3). Apr 13, 2022
@rknegjens rknegjens force-pushed the multiheap-2-rebase branch 3 times, most recently from 5a953e9 to 96063b8 Compare April 14, 2022 18:52
Enable the addition of heap space at runtime. Advantages:
  - The ESP32 has a fragmented heap so to use all of it the heap must be
    split.
  - Support a dynamic heap while running on an OS, adding more heap when
    necessary.

Original commit from Jan 2018. Rebased onto latest master.
@rknegjens rknegjens force-pushed the multiheap-2-rebase branch from 96063b8 to d0bbd2a Compare April 21, 2022 19:46
Use C macros to reduce size of firmware images when GC multiple heap
feature is disabled.

Image size diffs versus current master
(28e7e15) when multi-heap *disabled*:

       bare-arm:    +0 +0.000%
    minimal x86:    +0 +0.000%
       unix x64:   -16 -0.003%
    unix nanbox:   -20 -0.004%
          stm32:    -8 -0.002% PYBV10
         cc3200:    +0 +0.000%
        esp8266:    +8 +0.001% GENERIC
          esp32:    +0 +0.000% GENERIC
            nrf:   -20 -0.011% pca10040
            rp2:    +0 +0.000% PICO
           samd:    -4 -0.003% ADAFRUIT_ITSYBITSY_M4_EXPRESS

Note that only the stm32 image gets slightly bigger (+8).

Image size diffs versus current master when multi-heap is *enabled* with
`MICROPY_GC_MULTIHEAP=1` (but not extra code to add more heaps):

    unix x64: +1032 +0.197% [incl +544(bss)]
       esp32:  +592 +0.039% GENERIC[incl +16(data) +264(bss)]
When multiple GC heaps are enabled for the unix port, the value of the C
macro MICROPY_GC_N_HEAPS determines over how many heaps the requested
heap size is divided. The coverage variant sets this to 4 to cover the
`gc_add()` function and test the multiple heaps feature. The default
is 1.
@rknegjens rknegjens force-pushed the multiheap-2-rebase branch from d0bbd2a to 691fca8 Compare April 21, 2022 20:01
@peterhinch
Copy link
Contributor

Would this enable a fix for #8428?

@dpgeorge
Copy link
Member

Would this enable a fix for #8428?

Yes. The aim with multi-heap is that the system can add more dynamically. Eg on the unix port it can expand the heap as necessary. Same for any port that has lots of RAM.

@rknegjens
Copy link
Contributor Author

rknegjens commented Apr 25, 2022

When multi-heaps are enabled an array of pointers is used to track the heap areas that correspond to each block on the gc stack. This causes an increase in uninitialized memory (bss) in the image sizes . Pointers to heap areas are used because the heap areas themselves are stored using a linked list.

Firmware size diff for multi-heap enabled versus disabled when heap areas are stored with a linked list (the current PR):

   unix x64: +1048 +0.200% [incl +544(bss)]
      esp32:  +592 +0.039% GENERIC[incl +16(data) +264(bss)]

I've experimented with reducing the image sizes by storing the heap areas using an array instead of a linked list, so that only a byte array (uint8_t) is needed to index the heap areas for blocks on the gc stack (versus a pointer areay). While my array implementation reduces the bss contributions as expected, the total size actually goes up due to the slightly more involved code.

Firmware size diff for multi-heap enabled versus disabled when heap areas stored with an array:

   unix x64: +1512 +0.289% [incl +128(bss)]
      esp32:  +892 +0.059% GENERIC[incl +96(bss)]

It's likelythe array implementation can be optimised further (happy to share a link to this branch), but my impression is that the total gain will be minimal (if anything). Seeing as the firmware sizes across different ports when multi-heap is disabled essentially stay the same, I propose to merge the current (linked list) implementation.

@dpgeorge
Copy link
Member

Running performance test of this PR on PYBv1.0, the diff compared to master is:

diff of scores (higher is better)
N=100 M=100                     perf0 ->      perf1         diff      diff% (error%)
bm_chaos.py                    359.13 ->     367.33 :      +8.20 =  +2.283% (+/-0.00%)
bm_fannkuch.py                  76.65 ->      77.31 :      +0.66 =  +0.861% (+/-0.00%)
bm_fft.py                     2523.18 ->    2538.62 :     +15.44 =  +0.612% (+/-0.00%)
bm_float.py                   5705.78 ->    5888.59 :    +182.81 =  +3.204% (+/-0.01%)
bm_hexiom.py                    47.03 ->      46.92 :      -0.11 =  -0.234% (+/-0.00%)
bm_nqueens.py                 4384.86 ->    4399.35 :     +14.49 =  +0.330% (+/-0.00%)
bm_pidigits.py                 629.67 ->     628.62 :      -1.05 =  -0.167% (+/-0.52%)
core_import_mpy_multi.py       467.96 ->     469.93 :      +1.97 =  +0.421% (+/-0.00%)
core_import_mpy_single.py       57.89 ->      58.20 :      +0.31 =  +0.535% (+/-0.00%)
core_qstr.py                    63.40 ->      63.41 :      +0.01 =  +0.016% (+/-0.00%)
core_yield_from.py             361.18 ->     356.05 :      -5.13 =  -1.420% (+/-0.00%)
misc_aes.py                    406.91 ->     412.98 :      +6.07 =  +1.492% (+/-0.00%)
misc_mandel.py                3292.94 ->    3361.38 :     +68.44 =  +2.078% (+/-0.00%)
misc_pystone.py               2213.16 ->    2211.77 :      -1.39 =  -0.063% (+/-0.00%)
misc_raytrace.py               363.30 ->     373.80 :     +10.50 =  +2.890% (+/-0.00%)
viper_call0.py                 576.78 ->     575.11 :      -1.67 =  -0.290% (+/-0.13%)
viper_call1a.py                550.32 ->     549.88 :      -0.44 =  -0.080% (+/-0.07%)
viper_call1b.py                438.20 ->     435.90 :      -2.30 =  -0.525% (+/-0.12%)
viper_call1c.py                442.81 ->     441.18 :      -1.63 =  -0.368% (+/-0.13%)
viper_call2a.py                536.26 ->     535.40 :      -0.86 =  -0.160% (+/-0.16%)
viper_call2b.py                378.85 ->     377.42 :      -1.43 =  -0.377% (+/-0.19%)

@dpgeorge
Copy link
Member

Rebased and merged in bcc827d through d2e4cf0, with a rename of the new option to MICROPY_GC_SPLIT_HEAP.

This is a great feature, thank you @aykevl and @rknegjens !

@dpgeorge dpgeorge closed this Jul 22, 2022
@peterhinch
Copy link
Contributor

Hopefully this will lead to progress on #8428 (comment).

@tve
Copy link
Contributor

tve commented Nov 21, 2022

Is there any documentation on how any of this works? What do I need to enable? Is it enabled by default on which ports? What does it really do, e.g., what is the starting heap size? When does it add more heap segments? How can one limit this, e.g. reserve a specific amt of memory for other stuff, like ESP-IDF? Can heaps be freed ever?

@dpgeorge
Copy link
Member

Is there any documentation on how any of this works?

The documentation available at the moment is in the code, the use of this feature by the unix port (see ports/unix/main.c).

What do I need to enable?

Enable MICROPY_GC_SPLIT_HEAP in the config file.

Is it enabled by default on which ports?

Only unix coverage enables it.

What does it really do, e.g., what is the starting heap size?

The starting heap size is set when gc_init(...) is called by the port, as usual.

When does it add more heap segments?

When gc_add(...) is called by the port. It is not called automatically.

How can one limit this, e.g. reserve a specific amt of memory for other stuff, like ESP-IDF?

It's up to the port (C code) to implement this logic.

Can heaps be freed ever?

No.

@abuvanth
Copy link

@dpgeorge how to enable this for esp32

@dpgeorge
Copy link
Member

how to enable this for esp32

See my comment directly above on how to use this feature.

@rknegjens rknegjens deleted the multiheap-2-rebase branch March 26, 2023 03:14
tannewt pushed a commit to tannewt/circuitpython that referenced this pull request Oct 26, 2023
Fix typo in TCP server test readme.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
py-core Relates to py/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants