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

Avoid stack overflow when initializing HashBuffers. #164

Conversation

anforowicz
Copy link
Contributor

Before this commit, Box::default() that creates default HashBuffers would in debug mode create the default HashBuffers struct on the stack and then (as a separate step copy it) from the stack into the heap (i.e. into the Box). This would require 164098 bytes of stack space and would trigger stack overflow in some scenarios (e.g. when running Chromium tests built in debug-mode on an iOS device - see https://crbug.com/394824910).

I have tested that Chromium tests on iOS pass after changes from this commit - see https://crrev.com/c/6244910

Before this commit, `Box::default()` that creates default `HashBuffers`
would in debug mode create the default `HashBuffers` struct on the stack
and then as a separate step copy it into the heap (i.e. into the `Box`).
This would require 164098 bytes of stack space and would trigger stack
overflow in some scenarios (e.g. when running Chromium tests built in
debug-mode on an iOS device - see https://crbug.com/394824910).
@oyvindln
Copy link
Collaborator

oyvindln commented Feb 9, 2025

This might be related to the same problem that caused this - an upstream change in rust to Box::default resulted in that function causing more stack allocations in debug mode. I don't know if changing the code in flate2 to use Box::new instead would make any difference or not.

Will have to check if this change has any performance implications or not (granted that will be dwarfed by sorting the regression noted in #163 if that can be sorted for the next release in any case.)

@oyvindln
Copy link
Collaborator

oyvindln commented Feb 10, 2025

It doesn't seem to have any notable performance impact so I will just merge as is for now

@oyvindln oyvindln merged commit 921bc2c into Frommi:master Feb 10, 2025
8 checks passed
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.

2 participants