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

[4.2][Debug] Do not overwrite startTime. #4914

Merged
merged 1 commit into from
Jul 11, 2021
Merged

[4.2][Debug] Do not overwrite startTime. #4914

merged 1 commit into from
Jul 11, 2021

Conversation

sfadschm
Copy link
Contributor

@sfadschm sfadschm commented Jul 6, 2021

This PR gives a little change to the execution time collection, by only overwriting the startTime property of the CodeIgniter4 object, if it is not yet set.

This property gets first set here in the constructor:

$this->startTime = microtime(true);

After that, it gets overwritten from bootstrap.php during $app->run() via:

$this->startTime = microtime(true);

I am not sure what was the goal here. In the current state, we basically restart the "total execution" timer and possibly miss some ms (i.e., the time that $app->initialize(); takes). This is not really helpful I think.

As far as I can tell, the part now included in the if could be removed completely, as startTime will likely never be null at this point.

Checklist:

  • Securely signed commits
  • Conforms to style guide

@sfadschm
Copy link
Contributor Author

sfadschm commented Jul 6, 2021

Judging from @lonnieezell's (very old) commits, this has been some debate in the past:

https://github.com/codeigniter4/CodeIgniter4/commits/9521545bdce6a33f56e0f48767f4bf9855110e7d

However, in the current state, startTime will always be overwritten before it is used unless someone edits index.php and puts some content between the CodeIgniter4 app construction and $app->run().

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

I think this is fine. We can see if Lonnie weighs in but we have come a long ways and I don't think comparing internal benchmarking between CI3 and CI4 does anything useful.

FWIW I have plans for a benchmarking workflow. I will probably dig into this a bit more when I get to that.

@sfadschm
Copy link
Contributor Author

sfadschm commented Jul 7, 2021

Should I change to just remove the line?
I do not think the conditional will ever be true as the method cannot be run without the constructor being run beforehand.
Only way would be if someone intentionally resets startTime to null after the constructor.

@sfadschm
Copy link
Contributor Author

sfadschm commented Jul 7, 2021

FWIW I have plans for a benchmarking workflow. I will probably dig into this a bit more when I get to that.

Sounds great 😄

@paulbalandan paulbalandan merged commit 09bc563 into codeigniter4:4.2 Jul 11, 2021
@sfadschm sfadschm deleted the 4.2-starttime branch July 11, 2021 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants