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

[ticket/12687] Add a constant to display the load time without DEBUG #2570

Merged
merged 12 commits into from Jun 13, 2014

Conversation

@Nicofuma
Copy link
Member

commented Jun 10, 2014

https://tracker.phpbb.com/browse/PHPBB3-12687


In phpBB 3.0.x the DEBUG constant had a very little impact: track the sql queries and display the load time. And many user (maybe something like 70%) like to display the load time of the page to the user and so they set the DEBUG constant to true.

In 3.1.x the DEBUG constant add many other things:

  • twig debug
  • dump or not url_matcher
  • search or not the templates events on each page
  • ...

And so the impact of the DEBUG constant on the performances isn't negligible at all.

But the users still want to display the load time, and if we tell them to set the DEBUG constant to true they all will complain because their board will take something like 0.7s on each page (and for that the support team will want kill us xD)

So we need to managed the displaying of the load time information independently of the DEBUG constant.


Will be revert in 3.2 by ticket/12620 (#2521)

PHPBB3-12687

@@ -5074,7 +5074,7 @@ function page_footer($run_cron = true, $display_template = true, $exit_handler =
$debug_output = sprintf('Time : %.3fs | ' . $db->sql_num_queries() . ' Queries | GZIP : ' . (($config['gzip_compress'] && @extension_loaded('zlib')) ? 'On' : 'Off') . (($user->load) ? ' | Load : ' . $user->load : ''), $totaltime);
if ($auth->acl_get('a_') && defined('DEBUG'))
if ($auth->acl_get('a_') && defined('DISPLAY_LOAD_TIME'))

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 10, 2014

Contributor

I think the Explain link should only be available with defined('DEBUG')

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 10, 2014

Author Member

updated

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2014

As already discussed elsewhere this is okay for 3.1

In 3.2 we want to use enviroments instead of constants: #2521

@bantu

This comment has been minimized.

Copy link
Member

commented Jun 10, 2014

At least use a prefix for the constant. PHPBB_.

{
if ($memory_usage = memory_get_peak_usage())
if (function_exists('memory_get_peak_usage'))

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 10, 2014

Contributor

Merge with previous if?
if (defined('DISPLAY_LOAD_TIME') && function_exists('memory_get_peak_usage'))

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 10, 2014

Author Member

updated

}
}
$template->assign_vars(array(
'DEBUG_OUTPUT' => (defined('DEBUG')) ? $debug_output : '',
'DEBUG_OUTPUT' => (defined('PHPBB_DISPLAY_LOAD_TIME')) ? $debug_output : '',

This comment has been minimized.

Copy link
@bantu

bantu Jun 11, 2014

Member

Let's not do the same check here again. Define $debug_output as empty above, then just use that here.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 11, 2014

Author Member

updated

{
if (function_exists('memory_get_peak_usage'))
if (defined('PHPBB_DISPLAY_LOAD_TIME') && function_exists('memory_get_peak_usage'))

This comment has been minimized.

Copy link
@bantu

bantu Jun 11, 2014

Member

This is pointless right now.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 11, 2014

Author Member

updated

$debug_output .= ' | <a href="' . build_url() . '&amp;explain=1">Explain</a>';
if (defined('DEBUG'))
{
$debug_output .= ' | <a href="' . build_url() . '&amp;explain=1">Explain</a>';

This comment has been minimized.

Copy link
@bantu

bantu Jun 11, 2014

Member

Considering that this will be hidden when DEBUG is defined but not PHPBB_DISPLAY_LOAD_TIME, we might want to reconsider showing the time also when DEBUG is deinfed but PHPBB_DISPLAY_LOAD_TIME is not.

@bantu

This comment has been minimized.

Copy link
Member

commented Jun 11, 2014

I think this debug output thing needs a complete refactoring:

  • Deduplicate the generation (use a new function?)
  • Collect everything into an array first, then use joining on " | " to generate the debug string (especially because of the different options we are adding now).
  • Display Time when PHPBB_DISPLAY_LOAD_TIME is defined.
  • Display Queries when PHPBB_DISPLAY_LOAD_TIME is defined.
  • Display Peak Memory when PHPBB_DISPLAY_LOAD_TIME is defined.
  • Display GZIP when DEBUG is defined (this is rather useless information)
  • Display SQL Explain when DEBUG is defined
@Nicofuma

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2014

updated

@@ -5030,6 +5030,63 @@ function page_header($page_title = '', $display_online_list = false, $item_id =
}
/**
* Set the DEBUG_OUTPUT template var
*/
function display_debug_output()

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

prefix phpbb_

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

will have to remove the display_ prefix if this returns a string. need to find a better name then.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

updated

}
}

$template->assign_vars(array(

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

Don't depend on the template, but return a string instead?

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

updated

$debug_info[] = sprintf('Time : %.3fs', $totaltime);
$debug_info[] = $db->sql_num_queries() . ' Queries';

if ($auth->acl_get('a_'))

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

I don't think this is necessary. Just always show the memory consumption since it is also useful for when you are not logged in. If this gives an attacker useful information, it really is the admin's fault for defining PHPBB_DISPLAY_LOAD_TIME because they want to be cool.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

updated

{
if (function_exists('memory_get_peak_usage'))
{
if ($memory_usage = memory_get_peak_usage())

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

Remove inline assignment.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

updated


if ($auth->acl_get('a_'))
{
$debug_info[] = '<a href="' . build_url() . '&amp;explain=1">Explain</a>';

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

This should probably be SQL Explain.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

updated

$template->assign_vars(array(
'DEBUG_OUTPUT' => (defined('DEBUG')) ? $debug_output : '',

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

Keep this. Just call the function here that returns the string.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

updated

*/
function display_debug_output()
{
global $starttime, $template, $db, $config, $auth, $user, $request;

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

$template, $db, $config, $auth, $user, $request should probably be arguments. $starttime is a bit more tricky.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

i don't think it will change anything because only globals are used in the file. All of these functions have to be moved into helper classes and use the DI before phpBB 4.0 (i think) but it's not for now^^

{
global $starttime, $template, $db, $config, $auth, $user, $request;

if ($request->variable('explain', false) && $auth->acl_get('a_') && defined('DEBUG') && method_exists($db, 'sql_report'))

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

this should probably be in yet another function that gets called from page_footer and adm_page_footer instead

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

updateda

*/
function phpbb_check_and_display_sql_report()
{
global $request, $auth, $db;

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

Pass these as arguments.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

updated

*/
function phpbb_generate_debug_output()
{
global $starttime, $db, $config, $auth, $user;

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

Pass $db, $config, $auth, $user as arguments.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

updated

if (defined('PHPBB_DISPLAY_LOAD_TIME'))
{
$mtime = explode(' ', microtime());
$totaltime = $mtime[0] + $mtime[1] - $starttime;

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

Check for $starttime being defined. I guess we could also just use $GLOBALS['starttime'] instead of importing it via the global statement.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

updated

}

/**
* Set the DEBUG_OUTPUT template var

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

This is no longer true. Please add proper doc blocks to the two functions.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

updated

@bantu

This comment has been minimized.

Copy link
Member

commented Jun 12, 2014

This looks pretty good and should be ready soon. Thank you for your work.

PHPBB3-12687
@@ -5030,6 +5030,63 @@ function page_header($page_title = '', $display_online_list = false, $item_id =
}
/**
* Check and display the SQL report if requested.
*/
function phpbb_check_and_display_sql_report($request, $auth, $db)

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 12, 2014

Contributor

Type int the variables, then you can also remove method_exists($db, 'sql_report') as the method is part of the interface

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 12, 2014

Contributor

Also the parameters are missing from the doc block

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

updated

if (defined('PHPBB_DISPLAY_LOAD_TIME'))
{
$mtime = explode(' ', microtime());
$totaltime = $mtime[0] + $mtime[1] - $GLOBALS['starttime'];

This comment has been minimized.

Copy link
@MGaetan89

MGaetan89 Jun 12, 2014

Contributor

This can be $totaltime = microtime(true) - $GLOBALS['starttime'];, as phpBB3.1 supports PHP 5.3+.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 12, 2014

Author Member

indeed, updated

PHPBB3-12687
PHPBB3-12687
@nickvergessen

This comment has been minimized.

Copy link

commented on phpBB/includes/functions.php in d705547 Jun 12, 2014

Missing leading \ and should be request_interface

PHPBB3-12687
// Output page creation time
if (defined('PHPBB_DISPLAY_LOAD_TIME'))
{
$totaltime = microtime(true) - $GLOBALS['starttime'];

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

As I said, please make sure starttime is defined. I.e. wrap this into if isset $GLOBALS['starttime'].

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

Alternatively, we could use $_SERVER['REQUEST_TIME_FLOAT'] if that is basically always available.

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

REQUEST_TIME_FLOAT is PHP 5.4+

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 13, 2014

Author Member

updated

$debug_info[] = sprintf('Time : %.3fs', $totaltime);
$debug_info[] = $db->sql_num_queries() . ' Queries';

if (function_exists('memory_get_peak_usage'))

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

Maybe we should remove this too. memory_get_peak_usage is PHP 5.2+

This comment has been minimized.

Copy link
@MGaetan89

MGaetan89 Jun 13, 2014

Contributor

This is what I'm working on in #2404

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 13, 2014

Author Member

updated

$totaltime = microtime(true) - $GLOBALS['starttime'];

$debug_info[] = sprintf('Time : %.3fs', $totaltime);
$debug_info[] = $db->sql_num_queries() . ' Queries';

This comment has been minimized.

Copy link
@bantu

bantu Jun 12, 2014

Member

This could possibly extended to "x Queries (y cached)". I would probably find that useful.

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 13, 2014

Author Member

updated

{
$memory_usage = get_formatted_filesize($memory_usage);

$debug_info[] = 'Peak Memory Usage: ' . $memory_usage;

This comment has been minimized.

Copy link
@bantu

bantu Jun 13, 2014

Member

Space before ":" for consistency. Not sure how this will look, though.

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 13, 2014

Contributor

Better remove the other spaces?

This comment has been minimized.

Copy link
@bantu

bantu Jun 13, 2014

Member

I agree with nickvergessen

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 13, 2014

Author Member

updated


if ($user->load)
{
$debug_info[] = 'Load : ' . $user->load;

This comment has been minimized.

Copy link
@nickvergessen

nickvergessen Jun 13, 2014

Contributor

Remove all the french spaces please :P

This comment has been minimized.

Copy link
@Nicofuma

Nicofuma Jun 13, 2014

Author Member

updated

@bantu bantu merged commit 8e9b273 into phpbb:develop-ascraeus Jun 13, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
bantu added a commit that referenced this pull request Jun 13, 2014
[ticket/12687] Add a constant to display the load time without DEBUG

* Nicofuma/ticket/12687:
  [ticket/12687] Remove french spaces
  [ticket/12687] Display the number of cached queries
  [ticket/12687] Don't check if memory_get_peak_usage() exists
  [ticket/12687] Wrap $GLOBALS['starttime'] in a if
  [ticket/12687] Fix the namespace of $request
  [ticket/12687] Update doc block
  [ticket/12687] Use microtime(true)
  [ticket/12687] Remove globals
  [ticket/12687] Return a string and move the displaying of the sql report
  [ticket/12687] Rename DISPLAY_LOAD_TIME to PHPBB_DISPLAY_LOAD_TIME
  [ticket/12687] Display the explain link only when DEBUG is set
  [ticket/12687] Add a constant to display the load time without DEBUG
rxu added a commit to rxu/phpbb3 that referenced this pull request Jun 14, 2014
…nabled

PR phpbb#2570 has added new constant to display load time information without
debug mode is being on (https://tracker.phpbb.com/browse/PHPBB3-12687).
This patch expands the total load time info with SQL/PHP load times,
while hiding the additional info with <abbr> element.

PHPBB3-12704
rxu added a commit to rxu/phpbb3 that referenced this pull request Jun 14, 2014
…nabled

PR phpbb#2570 has added new constant to display load time information without
debug mode is being on (https://tracker.phpbb.com/browse/PHPBB3-12687).
This patch expands the total load time info with SQL/PHP load times,
while hiding the additional info with <abbr> element.

PHPBB3-12704
rxu added a commit to rxu/phpbb3 that referenced this pull request Jun 14, 2014
…nabled

PR phpbb#2570 has added new constant to display load time information without
debug mode is being on (https://tracker.phpbb.com/browse/PHPBB3-12687).
This patch expands the total load time info with SQL/PHP load times,
while hiding the additional info with <abbr> element.

PHPBB3-12704
rxu added a commit to rxu/phpbb3 that referenced this pull request Jun 20, 2014
…nabled

PR phpbb#2570 has added new constant to display load time information without
debug mode is being on (https://tracker.phpbb.com/browse/PHPBB3-12687).
This patch expands the total load time info with SQL/PHP load times,
while hiding the additional info with <abbr> element.

PHPBB3-12704
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.