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

perf: Change current directory in public/index.php only if necessary #7559

Merged
merged 1 commit into from
Jun 27, 2023
Merged

Conversation

bebehr
Copy link
Contributor

@bebehr bebehr commented Jun 11, 2023

Description
Checked if current directory is pointing to the front controller's directory.
Only change the directory if they differ because chdir call is not necessary and is not performant.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added the GPG-Signing needed Pull requests that need GPG-Signing label Jun 11, 2023
@kenjis
Copy link
Member

kenjis commented Jun 11, 2023

Thank you for sending this PR.

First of all, you must sign all your commits.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing

@kenjis
Copy link
Member

kenjis commented Jun 11, 2023

The chdir() came from: #36 and #5972

@kenjis
Copy link
Member

kenjis commented Jun 11, 2023

Only change the directory if they differ because chdir call is not necessary and is not performant.

Can you show the benchmarks?

@kenjis kenjis added the 4.4 label Jun 11, 2023
@bebehr
Copy link
Contributor Author

bebehr commented Jun 20, 2023

Running on Windows 11 with XAMPP
Loop 100000 times:

chdir(FCPATH);
4.0267419815063 seconds

if (getcwd() . DIRECTORY_SEPARATOR !== FCPATH) {}
0.030242919921875 seconds

Assuming the default case is: the workdir is already correct and does not have to be changed

@bebehr
Copy link
Contributor Author

bebehr commented Jun 22, 2023

Thank you for sending this PR.

First of all, you must sign all your commits. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing

Done

@bebehr
Copy link
Contributor Author

bebehr commented Jun 22, 2023

Only change the directory if they differ because chdir call is not necessary and is not performant.

Can you show the benchmarks?

#7559 (comment)

@kenjis
Copy link
Member

kenjis commented Jun 23, 2023

Thanks.

But see https://github.com/codeigniter4/CodeIgniter4/pull/7559/commits
The first commit is not signed.

And we don't use git merge in PR branches.
Can you rebase?
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

If you set up git to sign automatically, git rebase will sign your all commits.

@kenjis
Copy link
Member

kenjis commented Jun 24, 2023

See https://github.com/codeigniter4/CodeIgniter4/pull/7559/commits
You are merging again and again.

As I said, we do not use git merge, but use git rebase.

@kenjis kenjis removed the GPG-Signing needed Pull requests that need GPG-Signing label Jun 24, 2023
@bebehr
Copy link
Contributor Author

bebehr commented Jun 24, 2023

See https://github.com/codeigniter4/CodeIgniter4/pull/7559/commits You are merging again and again.

As I said, we do not use git merge, but use git rebase.

Thanks! Sorry, signing and rebase was completely new to me... I think I got it right now.

public/index.php Outdated Show resolved Hide resolved
@kenjis
Copy link
Member

kenjis commented Jun 24, 2023

On macOS.

chdir(FCPATH);
1.7629029750824 seconds

if (getcwd() . DIRECTORY_SEPARATOR !== FCPATH) {}
1.63840508461 seconds

$s = microtime(true);
for ($i = 0; $i <100000; $i++) {
    // Ensure the current directory is pointing to the front controller's directory
    if (getcwd() . DIRECTORY_SEPARATOR !== FCPATH) {
        chdir(FCPATH);
    }
}
echo microtime(true) - $s;
exit;

@kenjis
Copy link
Member

kenjis commented Jun 24, 2023

@BennyBPB Thank you for rebasing!

There are three commits but it seems okay with one commit.
If you can squash the commits, do it.

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

Remove the license comment.

@bebehr
Copy link
Contributor Author

bebehr commented Jun 24, 2023

Remove the license comment.

Done, file comment is removed.

@bebehr
Copy link
Contributor Author

bebehr commented Jun 24, 2023

@BennyBPB Thank you for rebasing!

There are three commits but it seems okay with one commit. If you can squash the commits, do it.

Removing the file comment was commit no 4.
I squished them into 1 commit now.

@bebehr bebehr requested a review from kenjis June 24, 2023 10:16
@kenjis kenjis changed the title Change current directory in public/index.php only if necessary perf: Change current directory in public/index.php only if necessary Jun 25, 2023
@kenjis
Copy link
Member

kenjis commented Jun 25, 2023

@BennyBPB Thank you for squishing!

@kenjis kenjis merged commit 1be4b4f into codeigniter4:4.4 Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants