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

Changed query bars. #3

Merged
merged 7 commits into from Jun 16, 2013
Merged

Changed query bars. #3

merged 7 commits into from Jun 16, 2013

Conversation

ghost
Copy link

@ghost ghost commented Jun 16, 2013

Added click-to-anchor on bars.
Added tooltips to bars.
Added time and memory bars to Profile Information
Changed html output of and added colored labels to Profile Information
Changed html output of and added labels to Memory Usage

See live demo: http://ndev.nl/debug

Peter van Westen added 2 commits June 16, 2013 16:57
Added click-to-anchor on bars.
Added tooltips to bars.
Added time and memory bars to Profile Information
Changed html output of and added colored labels to Profile Information
Changed html output of and added labels to Memory Usage
@ghost
Copy link
Author

ghost commented Jun 16, 2013

I added a $marks var to the libraries/joomla/profiler/profiler.php containing the separate mark data. And leaving the creation of the string (sprintf) to the debug plugin.
So I don't think the buffer (and getBuffer method) is used anymore. But I'll leave it in case other plugins/modules use it.
I think the creating of the html/strings should not be inside the profiler.php anyway.

@beat
Copy link
Owner

beat commented Jun 16, 2013

@nonumber Awesome! This will help well overall profiling, and brings the debug zone uptodate.

One minor issue:

  • The first query doesn't highlight the bar

A few last ideas that you could do before I merge the code from this awesome pull request:

    1. on the PHP profiling, change seconds to miliseconds for consistency, and page times should really be counted in ms instead of seconds.
    1. memory use: proposing to use orange for increases over 4 MB and red for increases over 6 MB.
    1. SQL queries: somehow mark the second time an identical query is executed (instead of being used from cache). It's not always an error, as sometimes it is justified to do several times same query. So not a red condition, but an orange one. We are already counting them, so it should be "easy". It could be e.g. a "Duplicate query" orange warning label after timing information.

Do you wish to do the things above as new commits within this pull request ?

@ghost
Copy link
Author

ghost commented Jun 16, 2013

Re: 2) The way I have it now is that anything that is more than 1.5 times the average is red. And anything less than 2/3 times (= divided by 1.5) is green. between that is orange.
Same for Time and Memory.

I think it should somehow be relative to the total time. Otherwise you will get everything green on fast servers and everything red on slow ones. Then coloring is useless.

@ghost ghost closed this Jun 16, 2013
@ghost ghost reopened this Jun 16, 2013
@beat
Copy link
Owner

beat commented Jun 16, 2013

Good point for "2)", even if I have no problem to mark all in red on crappy hostings... :D

@beat
Copy link
Owner

beat commented Jun 16, 2013

Database queries total time label could btw be colored as well, to attract attention to queries slownesses in the PHP profiling. ;)

@beat
Copy link
Owner

beat commented Jun 16, 2013

@nonumber : should i merge this pull request or wait ? :-)

Changed time notation from seconds to ms.
@ghost
Copy link
Author

ghost commented Jun 16, 2013

  1. Done
  2. Done

Regarding coloring for total DB time: what should we use as calculation? Probably best to compare it to the total page load time. What is a reasonable percentage? Above 25% = red, below 10% = green?

@beat
Copy link
Owner

beat commented Jun 16, 2013

Great!

Regarding coloring for total DB time: what should we use as calculation? Probably best to compare it to the total page load time. What is a reasonable percentage? Above 25% = red, below 10% = green?

Agree on principle and propose:

  • below 15% : Green
  • 15%-25%: Orange
  • 25%+: Red

@ghost
Copy link
Author

ghost commented Jun 16, 2013

Done :)

@beat
Copy link
Owner

beat commented Jun 16, 2013

Great! 👍

Are we now feature-complete ? :)
I can now merge ?

@beat
Copy link
Owner

beat commented Jun 16, 2013

Strange: In profile with 7.6 ms queries, it's green as it should.

But in top of Queries tab, it's red.

@ghost
Copy link
Author

ghost commented Jun 16, 2013

Think so :)
Life demo is up-to-date, btw: http://ndev.nl/debug/

@beat
Copy link
Owner

beat commented Jun 16, 2013

On your demo site too:

Database queries total: 18.0 ms (GREEN: Ok)
Memory Usage
Database Queries
61 Queries Logged 18.0 ms (RED: Not consistent)

@ghost
Copy link
Author

ghost commented Jun 16, 2013

Yeah, just saw that on my demo.
The total time is differently calculated. I'll fix...

@ghost
Copy link
Author

ghost commented Jun 16, 2013

Fixed?!

@beat
Copy link
Owner

beat commented Jun 16, 2013

Yup !

@nonumber That looks now like a really awesome tool. It spots and shows to the developer where the timings hurt most and why in a glance! It will now allow people to improve Joomla and extensions speed massively!

I'm now doing a last quick diff peer code review, then will press the merge button and post on tracker for asking for tests.

@ghost
Copy link
Author

ghost commented Jun 16, 2013

Cool.
I like how the duplicate query check turned out.
Pretty simple code-wise, indeed. But pretty valuable info too.
Like this is pretty worrying (on my localhost):
q
A 44 line (!) query being done 5 times!

@beat
Copy link
Owner

beat commented Jun 16, 2013

Yeah, noticed that on plain Joomla pages too.
A summary of duplicate queries at top when there are would help spot that too. Like e.g.: only displayed when there are duplicate queries:

40 Queries Logged 13.5 ms, (ORANGE)5 duplicate queries(/ORANGE)

@beat
Copy link
Owner

beat commented Jun 16, 2013

(edited comment above to orange)

@ghost
Copy link
Author

ghost commented Jun 16, 2013

So if 1 query is used 5 times, and another is used 3 times... is that 2 or 6 or 8 duplicates?!

@beat
Copy link
Owner

beat commented Jun 16, 2013

In summary it would be a total of 8 I would say. if needed a link on that label could scroll to anchor of first one.

@ghost
Copy link
Author

ghost commented Jun 16, 2013

Hmmm, if 1 query is used twice, then there is 1 duplicate, not 2, right?

@beat
Copy link
Owner

beat commented Jun 16, 2013

hm, right. So 5 times + 3 times means 4 times + 2 times too much = 6 duplicate queries.

But 8 is not incorrect either....Anyway any duplicate should be a warning. Now how much doesn't matter...

Main thing is that it's good to make it visible in orange at top.

@ghost
Copy link
Author

ghost commented Jun 16, 2013

How about this?

@beat
Copy link
Owner

beat commented Jun 16, 2013

Looks great !

Think it's more than time to quick-peer-review source and merge 👍

@beat
Copy link
Owner

beat commented Jun 16, 2013

Quick code review looks very fine.

While reviewing your very nice cleanups alll over the place, I noticed that function renderBacktrace was not using the formatLink() function which gives the clickable links with debug config in php.ini.

Would that also make sense ?

@beat
Copy link
Owner

beat commented Jun 16, 2013

I'm already merging this great pull request.

The renderBacktrace is used in case of PHP errors, so it's not directly related to SQL and profiling tools.

We can always add those nice little links later. ;-)

beat added a commit that referenced this pull request Jun 16, 2013
Merging awesome additions of @nonumber based on @beat and @roland-d's feedbacks to this completely re-styled and bootstraped joomla debug plugin.
@beat beat merged commit e4ead02 into beat:mysqlprofiling Jun 16, 2013
@ghost
Copy link
Author

ghost commented Jun 16, 2013

:)

beat added a commit that referenced this pull request Jun 26, 2013
beat pushed a commit that referenced this pull request Feb 17, 2016
beat pushed a commit that referenced this pull request Feb 17, 2016
beat added a commit that referenced this pull request Jan 21, 2022
…t of type int

Fixes `Deprecated: preg_split(): Passing null to parameter #3 ($limit) of type int is deprecated in libraries/src/Document/HtmlDocument.php on line 595`
beat added a commit that referenced this pull request Jan 23, 2022
…t of type int (joomla#36775)

Fixes `Deprecated: preg_split(): Passing null to parameter #3 ($limit) of type int is deprecated in libraries/src/Document/HtmlDocument.php on line 595`
beat pushed a commit that referenced this pull request Jan 25, 2022
…esumable-mod-1

[CMS PR 36708] Revert use DateInterval
HLeithner pushed a commit that referenced this pull request Jun 27, 2022
PHP Deprecated:  str_replace(): Passing null to parameter #3 ($subject)
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.

1 participant