Skip to content

Remove unused stack space. #13095

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Apr 10, 2025

I could not see the point of this code, so it looks like we can remove it.

It was introduced in 87e1616

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

thread_pthread.c:1948

  • The compile-time check for PTHREAD_STACK_DEFAULT size has been removed; please ensure that the default thread stack size is validated elsewhere or that this removal won't allow insufficient stack sizes in some environments.
# if PTHREAD_STACK_DEFAULT < RUBY_STACK_SPACE*5

thread_pthread.c:2054

  • By eliminating the reserved space calculation, the thread now uses the full stack size; please verify that this change does not increase the risk of stack overflow or negatively interfere with stack overflow detection logic.
nt->machine_stack_maxsize = stack_size;

thread_pthread.c:3150

  • The removal of the division by RUBY_STACK_SPACE_RATIO in the stack overflow detection may affect overflow behavior; confirm that the modified logic still correctly detects stack overflows.
size /= RUBY_STACK_SPACE_RATIO;

@ioquatix ioquatix requested a review from nobu April 10, 2025 10:54

This comment has been minimized.

@ioquatix ioquatix requested a review from ko1 April 10, 2025 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant