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

[Debug] Sort timeline elements by start and duration. #4863

Closed
wants to merge 25 commits into from
Closed

[Debug] Sort timeline elements by start and duration. #4863

wants to merge 25 commits into from

Conversation

sfadschm
Copy link
Contributor

This PR implements a sorting algorithm to the elements of the debug toolbar timeline.
Such sorting seems to be intended (and is useful) but has likely been forgotten:

// Sort it
return $data;

In principle, many of the elements are pre-sorted by nature, but chaos grows when multiple collectors are active (e.g. from Modules etc.).

I decided to sort the elements ascending by start time and then descending by duration time.
I.e., if a timed portion of code calls another timed portion of code and the timer start times happen to be equal (possibly unlikely, but just in case), the "outer" timer will be displayed first in the timeline.

This sorting also prepares for a possible "collapsible" timeline, which I need to think about how to realize.

Open for other opinions/ideas 😄

Forum link for discussion.

Timeline before changes:
Before

Timeline after changes:
After

Note that due to the styling of the single timeline elements (a left-right-padding of 5px), some very small elements appear to still be shifted on the first glance, but they're not if one puts a ruler on it 😆

Checklist:

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

@samsonasik samsonasik requested a review from MGatner June 24, 2021 04:46
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

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

I like the "after" changes as I think this is what a timeline should be, ...chronologically sorted. I'm just wondering though why there are gaps in between as I expected this to be like a Gantt chart with more or less overlapping bars. Does that mean not all places in the booting are timed?

@sfadschm
Copy link
Contributor Author

I'm just wondering though why there are gaps in between

Was wondering the same. I have to check. Might be some 'pre_system' events that I registered.

Gantt chart with more or less overlapping bars.

In principle the bars would not overlap at all. The styling however applies a horizontal padding of 2*5px to achieve the 4px border-radius.

@paulbalandan
Copy link
Member

Well, yes, I mean not technically overlapping. But the end of one bar is aligned with the start of the next bar on the next line.

@sfadschm
Copy link
Contributor Author

In that case, yes 😄
I just checked, it's not the pre_system events, they're included in the bootstrap timer.
In my case, it is the before filters in handleRequest()

@paulbalandan
Copy link
Member

Can you target 4.2 instead?

@sfadschm sfadschm changed the base branch from develop to 4.2 June 24, 2021 09:12
@sfadschm
Copy link
Contributor Author

Well, yeah, this went down the drain 😆

@sfadschm
Copy link
Contributor Author

Just for future reference: Is there a proper way to switch the target branch? Both switching it here in the PR and rebasing the local repo to 4.2 resulted in additional commits.

@paulbalandan
Copy link
Member

I think you need to branch off first from 4.2 branch for your PR branch whenever it is for content changes.

@MGatner
Copy link
Member

MGatner commented Jun 24, 2021

Probably beat to start over here. You want something like this (assuming you have your fork as origin and the main repo as upstream):

git fetch upstream
git switch -c sort-debugbar-timeline upstream/4.2

@MGatner MGatner closed this Jun 24, 2021
@sfadschm sfadschm deleted the sort-debugbar-timeline branch June 24, 2021 15:22
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.

4 participants