Skip to content

compiler: move allocations > 256 bytes to the heap #2628

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

Merged
merged 1 commit into from
Feb 11, 2022

Conversation

dgryski
Copy link
Member

@dgryski dgryski commented Feb 11, 2022

No description provided.

@aykevl
Copy link
Member

aykevl commented Feb 11, 2022

What's the rationale for this change?
(I'm not against it, but didn't expect this to be an issue anywhere).

@dgryski
Copy link
Member Author

dgryski commented Feb 11, 2022

In the snappy encoder on wasi, it tries to allocate a 32kb object on a 16k stack and the subsequent memset() call wipes out some globals, including metadataStart, corrupting the garbage collector: https://github.com/golang/snappy/blob/master/encode_other.go#L130

@aykevl
Copy link
Member

aykevl commented Feb 11, 2022

I see. I just read the #tinygo-dev discussion :)

It does seem reasonable to me to do this: a 32kB stack object is just way too large. 256 bytes is maybe a little bit on the low side? I think it depends on the target and the amount of stack/heap allocated for it (256 bytes is a lot on AVR, but 1kB would be perfectly fine on x86).

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

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

Let's just merge this as it does fix a bug.

@dgryski dgryski force-pushed the dgryski/move-to-heap branch from 88cd5b6 to f466c55 Compare February 11, 2022 16:53
@deadprogram
Copy link
Member

Thank you @dgryski now merging.

@deadprogram deadprogram merged commit 4b2edc9 into tinygo-org:dev Feb 11, 2022
@dgryski dgryski deleted the dgryski/move-to-heap branch February 14, 2022 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants