Skip to content

Conversation

@sfadschm
Copy link
Contributor

@sfadschm sfadschm commented Jul 1, 2021

This PR adds timers for the before and after filters.

During #4863, I found that significant gaps in the timeline can be produced by the filters.
As far as I could find, these are the only parts missing in the benchmarking.

For now, I opted to

  • start the timers in CodeIgniter4::handleRequest,
  • not divide the filter timers further into the individual filters.

In the current state, the Filters timer is displayed even if no filters are defined for the $position, this is not really the best solution in my opinion, however, $filters->run is called each time anyway, so why not log those ms, too.

The alternative would be to run $filters->initialize($uri) in handelRequest and then check the $filters->filtersClass property for defined filters.

On another note, I would like to explicitly time each running filter, too. However, the only way to achieve this currently would be to use the timer() helper in Filters::run() or to inject Services::timer into the Filters. Not sure if any of these options is feasible right now.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Conforms to style guide

@sfadschm sfadschm changed the title [Debug] Add benchmarking for filters. [4.2][Debug] Add benchmarking for filters. Jul 1, 2021
@sfadschm sfadschm marked this pull request as draft July 2, 2021 18:57
@sfadschm
Copy link
Contributor Author

sfadschm commented Jul 2, 2021

After considerations, this is a functioning and easy implementation, but using an own collector or such might be more useful. Will think about useful ways.

@sfadschm
Copy link
Contributor Author

sfadschm commented Jul 4, 2021

Updated to not use the helper workaround for general benchmarking.

@sfadschm sfadschm marked this pull request as ready for review July 4, 2021 16:36
@paulbalandan paulbalandan merged commit b17ec97 into codeigniter4:4.2 Jul 11, 2021
@sfadschm sfadschm deleted the add-filters-timer branch July 11, 2021 19:29
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.

2 participants