Skip to content

Add the ASYNCIFY_STACK_SIZE to the MEM_SIZE_SETTINGS #23357

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 9, 2025
Merged

Add the ASYNCIFY_STACK_SIZE to the MEM_SIZE_SETTINGS #23357

merged 3 commits into from
Jan 9, 2025

Conversation

@oleg-derevenetz
Copy link
Contributor

@oleg-derevenetz oleg-derevenetz commented Jan 9, 2025

Currently it is impossible to use kb/mb/gb for ASYNCIFY_STACK_SIZE, although, in theory, it should work.

sbc100
sbc100 approved these changes Jan 9, 2025
View reviewed changes
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm! Maybe we should add a test though?

@oleg-derevenetz
Copy link
Contributor Author

oleg-derevenetz commented Jan 9, 2025

Hi @sbc100

lgtm! Maybe we should add a test though?

Unfortunately, I'm not familiar with your test infrastructure. Is there any specific place where such a test should be added? I have not yet been able to find a place where these size suffixes would be tested at all.

@sbc100
Copy link
Collaborator

sbc100 commented Jan 9, 2025

How about adding self.set_setting('ASYNCIFY_STACK_SIZE', ['64kb']) to test_asyncify_advise in test/test_other.py, perhaps with a comment that this just to test that size can be specified in kb?

@oleg-derevenetz
Copy link
Contributor Author

oleg-derevenetz commented Jan 9, 2025

How about adding self.set_setting('ASYNCIFY_STACK_SIZE', ['64kb']) to test_asyncify_advise in test/test_other.py, perhaps with a comment that this just to test that size can be specified in kb?

Sure, thank you!

sbc100
sbc100 reviewed Jan 9, 2025
View reviewed changes
tools/settings.py Outdated
'MAXIMUM_MEMORY',
'DEFAULT_PTHREAD_STACK_SIZE'
'DEFAULT_PTHREAD_STACK_SIZE',
'ASYNCIFY_STACK_SIZE'
Copy link
Collaborator

@sbc100 sbc100 Jan 9, 2025

Choose a reason for hiding this comment

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

Can you add a trailing comma here so future additions can just be single line change?

Copy link
Contributor Author

@oleg-derevenetz oleg-derevenetz Jan 9, 2025

Choose a reason for hiding this comment

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

Sure, done.

@sbc100 sbc100 enabled auto-merge (squash) January 9, 2025 19:35
@sbc100 sbc100 merged commit dde19fa into emscripten-core:main Jan 9, 2025
29 checks passed
@oleg-derevenetz oleg-derevenetz deleted the asyncify_stack_size-mem_base_settings branch January 9, 2025 20:24
@dschuff dschuff mentioned this pull request Mar 14, 2025
Merged
@dschuff dschuff mentioned this pull request Mar 29, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbc100 sbc100 sbc100 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@oleg-derevenetz @sbc100