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

feat(runtime): Prevent stack overflow while collecting large lists #2248

Merged
merged 3 commits into from
Feb 17, 2025

Conversation

spotandjake
Copy link
Member

@spotandjake spotandjake commented Feb 17, 2025

Summary

This pr prevents stack overflows during the collection of large and extremely large lists.

Notes

  • Smallest program size decreased because of increased code sharing in decRefChildren
  • All data structures now support extremely deep nested objects in the final position.

Problem

While working on #2247 I noticed that decRef was overflowing the stack with lists that were only around 1350 elements.

Solution (For those interested)

The reason for this is that decRef and decRefChildren were not tco optimized, so the two options were to either handle lists separately or find a way to optimize decRef for tail calls. I decided on the second approach as it is more generic and provides improvements for other data structures as well.

The way I went about this was with a slightly cursed but valid modification where we free the parent before the children, this only works because we immediately decRef the children and when we free we don't zero the memory but instead just merge the blocks which will leave our child pointers unaffected. When freeing children we free most of the children in a loop and then free the final child outside of the loop in the tail position of decRefChildren allowing for the tco optimization. Initially this bloated our smallest program size by 100 bytes but I noticed that we are doing effectively the same operations for all dataTypes so I combined everything into a shared helper function that actually reduced our bundle size by 47 bytes.

Why not handle lists separately

Handling lists separately would have certainly been easier and if we are not up for the changes I made here it is still a viable approach however, it would add overhead to every ADT decRef and likely result in a hundred or so additional bytes being added to our smallest program size.

Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

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

Can you explain what parentPtr is for? It's not obvious to me reading through the code.

@spotandjake
Copy link
Member Author

Can you explain what parentPtr is for? It's not obvious to me reading through the code.

Essentially Memory.decRef has the behaviour that it returned the pointer passed in, I needed to find a way to preserve this behaviour through the tco optimization which required passing the parentPtr through decRef and decRefChildren, otherwise we wouldn't be able to preserve the behaviour while putting decRefChildren in the tail position.

@ospencer
Copy link
Member

That behavior isn't used by decRef or decRefChildren at all; you could just return it in the function that's exported at the end, e.g. let decRef = userPtr => { decRef(userPtr, false); userPtr }.

@ospencer
Copy link
Member

The internal decRef can just return void.

@spotandjake
Copy link
Member Author

The internal decRef can just return void.

I made that change, that simplifies the pointer handling semantics quite a bit.

Copy link
Member

@ospencer ospencer left a comment

Choose a reason for hiding this comment

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

Nice.

@ospencer ospencer enabled auto-merge February 17, 2025 03:59
@ospencer ospencer added this pull request to the merge queue Feb 17, 2025
Merged via the queue into grain-lang:main with commit 097ae7d Feb 17, 2025
12 checks passed
@github-actions github-actions bot mentioned this pull request Feb 17, 2025
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.

2 participants