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

Fix bugzilla 24524: Very slow process fork if RLIMIT_NOFILE is too high #9077

Merged
merged 1 commit into from
Nov 1, 2024

Conversation

schveiguy
Copy link
Member

Replaces #8990, refactored after merge of #9048

Credit to @trikko

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @schveiguy!

Bugzilla references

Auto-close Bugzilla Severity Description
24524 enhancement Very slow process fork if RLIMIT_NOFILE is too high

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#9077"

std/process.d Outdated Show resolved Hide resolved
@schveiguy
Copy link
Member Author

schveiguy commented Oct 30, 2024

@LightBender please don't auto merge before allowing others to review.

std/process.d Outdated Show resolved Hide resolved
@@ -1106,6 +1155,7 @@ private Pid spawnProcessPosix(scope const(char[])[] args,
// Until we find a way to perform this check we will try to use dlsym to
// check for the function. See: https://github.com/dlang/phobos/pull/9048
version (CRuntime_Glibc)
{
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI, this confused me, because of the block declaration inside. Using version/else without braces always bugs me.

@schveiguy schveiguy force-pushed the new8990 branch 2 times, most recently from 6e9dd6c to e1da63d Compare October 30, 2024 02:04
Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

LGTM


int fd = atoi(cast(char*) entry.d_name);

// Don't close stdin, stdout, stderr, or the directory file descriptor
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of date comment

Copy link
Member Author

Choose a reason for hiding this comment

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

Meh, effectively it's correct.

@the-horo
Copy link
Contributor

Some projects are more picky with how they want their commit history to look like so, given that @schveiguy's changes were pretty minimal on top of @trikko's, can the commit be reworded to reference the original author and include @schveiguy as a Co-authored-by: . Github will also pretty format that information, for example: the-horo@27082c1.

Also, if there's any relevant information surrounding the commit (like the fact that this is a refactor of a another PR) can it be included in the commit message to make it easier to find it in the future?

std/process.d Show resolved Hide resolved
@schveiguy
Copy link
Member Author

I don't need credit here, I didn't really do much. But if one looks at the PR you can see the credit via the description and the references.

I've spent enough time on this, if someone cares to make the changes, go ahead.

@LightBender LightBender merged commit 2a730ad into dlang:master Nov 1, 2024
10 checks passed
@schveiguy schveiguy deleted the new8990 branch November 1, 2024 17:47
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.

6 participants