Skip to content

Conversation

agatti
Copy link
Contributor

@agatti agatti commented Aug 19, 2025

Summary

This PR changes the "viper_ptr*_store_boundary" tests to make them fail more gracefully in low memory conditions.

The original version of the tests compiled viper code blocks on the fly when it needed them, making them fail at runtime on some boards that do not come with enough memory for this test. This clashes with "run-tests.py"'s ability to look for a particular signature to mark tests as skipped due to not enough memory.

Now compiled code blocks are generated at the beginning of the test inside an appropriate exception handler. In case of a memory error when pre-compiling a code block, the running test exits reporting a low memory condition to the test runner. This allows to have clean test runs on all platforms when it comes to viper pointer tests.

Testing

Modified tests were run on an ESP8266 without any parameters to make sure they were reported as too large, and then with an appropriate prologue file to make sure the output is correct. The same tests were also executed on the Unix port on x64, just to be sure.

Trade-offs and Alternatives

I've tried to simplify certain parts of the test to counter the added complexity of generating blocks upfront, although it came with a test output data rearrangement to make it work. On the other hand, now those tests share most of their code except for the typed getters and setters.

Copy link

codecov bot commented Aug 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.38%. Comparing base (1df5ee1) to head (b0fd007).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #17955   +/-   ##
=======================================
  Coverage   98.38%   98.38%           
=======================================
  Files         171      171           
  Lines       22296    22296           
=======================================
  Hits        21937    21937           
  Misses        359      359           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dpgeorge dpgeorge added the tests Relates to tests/ directory in source label Aug 19, 2025
@agatti
Copy link
Contributor Author

agatti commented Aug 19, 2025

This is odd. The error reported by Octoprobe is for an empty line in the source file, but that's right above a @micropython.viper decorator so I assume that's what it really wanted to mark.

That said, I thought compile-time memory errors were caught by the test runner, but no worries. I can move the declaration for set_index in an exec block and move the definition for buffer in the same try..catch block as the runtime compiled functions.

And if the remaining exec calls still make it fail, heh, I'll just wrap the whole lot in an exception handler and call it a day :) I'll fix this right away.

@agatti agatti force-pushed the viper-tests-memoryerror-handling branch from 4510716 to 65779c6 Compare August 22, 2025 13:29
@agatti
Copy link
Contributor Author

agatti commented Aug 22, 2025

The low memory errors you see on that specific board should be resolved now: https://reports.octoprobe.org/github_selfhosted_testrun_267/octoprobe_summary_report.html

On that board memory was so low that preallocating code blocks and the work buffer didn't leave anything for the exec calls to invoke the code block themselves. I'm not proud of the workaround being used but it works :)

try:

@micropython.viper
def set_index(dest: ptr16, i: int, val: uint):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't think this needs to be within the try-except. The memory for the native code is allocated during the compile phase (as you originally guessed). The only thing that's allocated at runtime (after the point where run-tests.py can detect an OOM failure) is the function-object wrapper, which is typically 1 GC block. The bytecode looks something like this:

...
00 MAKE_FUNCTION <address of native function>
02 STORE_NAME set_index
...

That's it. The small allocation is in the MAKE_FUNCTION opcode, and potentially a resize of the globals dict in STORE_NAME.

If this function can live where it was (outside the try-except) that would be preferable.

I think it's just the allocation of buffer that's the biggest issue, it's 8k by my calculations. So that definitely needs to be within the try-except.

Copy link
Contributor Author

@agatti agatti Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! My understanding was that the native compilation step happened at a later stage than it really does and thus any error could be caught that way, more like a missing import could be a failure the code can possibly recover from.

This makes me assume that, if the system is critically low on memory for other reasons and a parse + native compilation step is triggered from the main script there is no chance for the running environment to recover from this situation. Or, in other words, there's no MemoryError exception being raised and the system could possibly be caught in an unrecoverable bootloop.

If my understanding is correct, that's kind of scary, because all it takes is a memory preallocation in native code for example, driven by a data file whose header length got corrupted by a flash failure or by any other unexpected value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me assume that, if the system is critically low on memory for other reasons and a parse + native compilation step is triggered from the main script there is no chance for the running environment to recover from this situation.

I don't think that's true. There should always be a way to catch all Python errors (unless it happens compiling boot.py or main.py...).

Wherever you trigger a parse+compile step, you should always be able to wrap that in a try-except.

@agatti agatti force-pushed the viper-tests-memoryerror-handling branch from 65779c6 to 028dacf Compare August 25, 2025 20:42
@agatti
Copy link
Contributor Author

agatti commented Aug 25, 2025

I've resized the allocated buffers down to 5KiB each from the original 8KiB, keeping the same test behaviour.

This should also help on boards that are even more memory constrained - in fact these tests are not marked as too large neither on an ITSYBITSY M0+ nor on an ESP8266 now! (See Octoprobe runs 276 and 277, respectively)

This commit changes the "viper_ptr*_store_boundary" tests to make them
fail more gracefully in low memory conditions.

The original version of the tests compiled viper code blocks on the fly
when it needed them, making them fail at runtime on some boards that do
not come with enough memory for this test.  This clashes with
"run-tests.py"'s ability to look for a particular signature to mark
tests as skipped due to not enough memory.

Now compiled code blocks are generated at the beginning of the test
inside an appropriate exception handler.  In case of a memory error when
pre-compiling a code block, the running test exits reporting a low
memory condition to the test runner.  This allows to have clean test
runs on all platforms when it comes to viper pointer tests.

Signed-off-by: Alessandro Gatti <a.gatti@frob.it>
Copy link
Member

@dpgeorge dpgeorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good now, I tested locally on ItsyBitsy M0 and these tests run and pass.

@dpgeorge dpgeorge force-pushed the viper-tests-memoryerror-handling branch from 028dacf to b0fd007 Compare August 26, 2025 00:53
@dpgeorge dpgeorge merged commit b0fd007 into micropython:master Aug 26, 2025
30 of 31 checks passed
@dpgeorge
Copy link
Member

  • in fact these tests are not marked as too large neither on an ITSYBITSY M0+ nor on an ESP8266 now!

Actually, they are still too large on ESP8266, the Octoprobe output is:

...
pass  micropython/viper_misc_intbig.py 
pass  micropython/viper_ptr16_load.py 
pass  micropython/viper_ptr16_load_boundary.py 
pass  micropython/viper_ptr16_store.py 
lrge  micropython/viper_ptr16_store_boundary.py
pass  micropython/viper_ptr32_load.py 
pass  micropython/viper_ptr32_load_boundary.py 
pass  micropython/viper_ptr32_store.py 
lrge  micropython/viper_ptr32_store_boundary.py
pass  micropython/viper_ptr8_load.py 
pass  micropython/viper_ptr8_load_boundary.py 
pass  micropython/viper_ptr8_store.py 
lrge  micropython/viper_ptr8_store_boundary.py
pass  micropython/viper_storeattr.py 
pass  micropython/viper_subscr.py 
pass  micropython/viper_subscr_multi.py 
pass  micropython/viper_try.py 
pass  micropython/viper_types.py 
pass  micropython/viper_unop.py 
pass  micropython/viper_with.py 
84 tests performed (549 individual testcases)
84 tests passed
9 tests skipped: micropython/builtin_execfile.py micropython/const_math.py micropython/heap_locked.py micropython/heapalloc_bytesio2.py micropython/import_mpy_native_gc.py micropython/meminfo.py micropython/memstats.py micropython/native_fun_attrs_code.py micropython/ringio_big.py
6 tests skipped because they are too large: micropython/viper_args.py micropython/viper_binop_arith.py micropython/viper_large_jump.py micropython/viper_ptr16_store_boundary.py micropython/viper_ptr32_store_boundary.py micropython/viper_ptr8_store_boundary.py

I also verified that locally.

@agatti agatti deleted the viper-tests-memoryerror-handling branch August 26, 2025 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Relates to tests/ directory in source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants