Skip to content

Conversation

@tenderlove
Copy link
Member

We already have the open file descriptor, so we can avoid the overhead of resolving the filepath (as well as the overhead inside FileUtils) by just calling chmod on the file descriptor itself.

What was the end-user or developer problem that led to this PR?

Installing gems isn't as fast as I would like it to be, so I'm trying to speed it up.

What is your fix for the problem, implemented in this PR?

I have a branch that speeds up unpacking gems by about 20%, but it's a bunch of changes so I am trying to upstream the changes piecemeal so it's easier to review. The benchmark I'm using is here, it's just testing unpacking the gems a new Rails app depends on.

This PR changes FileUtils.chmod for File#chmod. Since we already have the file descriptor for the files we're writing to, we can avoid the overhead of FileUtils as well as use fchmod system call vs regular chmod. chmod has to find the file, but since we already have a handle we may as well use it.

Make sure the following tasks are checked

We already have the open file descriptor, so we can avoid the overhead
of resolving the filepath (as well as the overhead inside `FileUtils`)
by just calling `chmod` on the file descriptor itself.
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Thanks @tenderlove, makes sense!

I had attempted this same improvement in the past, but gave up after seeing the permission issues because I missed adding flush before chmod ❤️

@tenderlove
Copy link
Member Author

@deivid-rodriguez thanks! Do you mind merging? I don't have permission to commit to this repo anymore or I'd do it (unless there's something else we need to wait for, I don't want to presume 😅)

@deivid-rodriguez
Copy link
Contributor

No problem! I don't usually merge immediately but try to let PRs sit one or two days so other maintainers have the opportunity to chime in in case I missed something.

But happy to merge if you need it, chiming in after merge is perfectly ok too.

@deivid-rodriguez deivid-rodriguez merged commit 8072c8b into ruby:master Sep 12, 2025
73 checks passed
@tenderlove tenderlove deleted the file-chmod branch September 12, 2025 11:04
@tenderlove
Copy link
Member Author

No problem! I don't usually merge immediately but try to let PRs sit one or two days so other maintainers have the opportunity to chime in in case I missed something.

But happy to merge if you need it, chiming in after merge is perfectly ok too.

Understood. I just wanted to make sure you weren't waiting on me 😅

Copy link
Contributor

@simi simi left a comment

Choose a reason for hiding this comment

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

@deivid-rodriguez just for the protocol ✅

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.

3 participants