Skip to content
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

Slim down stack frames #8071

Merged
merged 1 commit into from
Jun 7, 2023
Merged

Slim down stack frames #8071

merged 1 commit into from
Jun 7, 2023

Conversation

tannewt
Copy link
Member

@tannewt tannewt commented Jun 6, 2023

This reduces the stack frame size of mp_builtin___import__ by limiting the support path length of files from 256 to 96. This function can be called recursively for nested imports so it adds up.

Also reduce mp_execute_bytecode (vm.c) from 206 a bc call to 124. This too is recursive and adds up. It is reduced by preventing some inlining. It may decrease performance slightly when importing and unpacking.

Adds two new scripts for debugging. One is used from gdb to print frame sizes in a backtrace. The other prints what pcs use a particular stack offset. This helps find infrequently used stack space.

Fixes #8053.

This reduces the stack frame size of mp_builtin___import__ by
limiting the support path length of files from 256 to 96. This
function can be called recursively for nested imports so it adds up.

Also reduce mp_execute_bytecode (vm.c) from 206 a bc call to 124.
This too is recursive and adds up. It is reduced by preventing
some inlining. It may decrease performance slightly when importing
and unpacking.

Adds two new scripts for debugging. One is used from gdb to print
frame sizes in a backtrace. The other prints what pcs use a
particular stack offset. This helps find infrequently used stack
space.

Fixes micropython#8053.
Copy link
Member

@jepler jepler left a comment

Choose a reason for hiding this comment

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

Very nice. There is a gcc flag to issue a diagnostic when a stack frame exceeds a given size, maybe we want to use it (and mark select functions as being permitted bigger stacks) but doing so might need a lot of annotations, I mention it just as an aside

@RetiredWizard
Copy link

I loaded this on a CPX and the test script from the issue does now run. Both this solution and the test I made of turning off CP modules seems to resolve the issue by making more memory available for (I'm guessing) the C-stack. Do you believe the "maximum recursion depth exceeded" message is actually the correct error and the issue was simply caused by starving the stack?

@bill88t
Copy link

bill88t commented Jun 7, 2023

I would request that this is not merged immediately.
I want to run extensive tests onto it.

@tannewt
Copy link
Member Author

tannewt commented Jun 7, 2023

I loaded this on a CPX and the test script from the issue does now run. Both this solution and the test I made of turning off CP modules seems to resolve the issue by making more memory available for (I'm guessing) the C-stack. Do you believe the "maximum recursion depth exceeded" message is actually the correct error and the issue was simply caused by starving the stack?

I couldn't really determine what change pushed this over the edge. The 8.0.0 release binaries work for me but building 8.0.0 with a newer gcc doesn't work. I suspect it depends on inlining decisions that different compiler versions make. I couldn't attribute it to library changes either.

I think this is a fine error. It matches what you'd get in cpython if you happened to hit stack limits.

I would request that this is not merged immediately. I want to run extensive tests onto it.

I'd prefer the opposite. Merging makes it easier for more people to test through S3 builds and pre-releases. We can always follow up with more changes if you find issues.

@tannewt tannewt merged commit b71f394 into adafruit:main Jun 7, 2023
@Neradoc
Copy link

Neradoc commented Jun 7, 2023

I couldn't really determine what change pushed this over the edge. The 8.0.0 release binaries work for me but building 8.0.0 with a newer gcc doesn't work. I suspect it depends on inlining decisions that different compiler versions make. I couldn't attribute it to library changes either.

@tannewt When I tried a bisect I ended at this commit, which looks like changes in inlining, but there are other things, because reverting it in latest did not help: 1108243

@tannewt
Copy link
Member Author

tannewt commented Jun 8, 2023

That would definitely make sense. Thanks for doing a bisect too! I did one but forgot to update submodules along the way and use a newer compiler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CPX library doesn't work on CPX (recursion depth hit)
5 participants