Bug Report #1843

Benchmark Bug

Added by Levi Stanley almost 4 years ago. Updated over 3 years ago.

Status:Closed Start date:07/15/2009
Priority:Low Due date:
Assignee:Isaiah DeRose-Wilson % Done:

100%
Category:Core
Target version:2.4
Resolution:fixed Points:

Description

I was looking threw the code, and noticed a weird situation with the Benchmark object.

It doesn't follow the documentation:

Code Snippet I tested.

...
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
Benchmark::start('test');
sleep(10);
print_r( Benchmark::get(true));

...

Output:

Array
(
[test] => Array
(
[time] => -32,439,853,747.1945
[memory] => -4015840
[count] => 27
)
)

I can see if the time and memory is negative, or count is greater than 1, then there may be nesting of benchmark on the same key. If this is the right logic, then the documentation needs to be revised for this.

I have attached a patch that cleans up the logic better, and is more efficient on duplicate cases if they exists.

Benchmark.php.patch (3 kB) Levi Stanley, 07/15/2009 09:10 pm

Associated revisions

Revision 4541
Added by Isaiah DeRose-Wilson over 3 years ago

Throw an exception when you try to start a benchmark that is already started. Fixes #1843

History

#1
Updated by Miodrag Tokić almost 4 years ago

As I understand benchmark class, you should put stop after start method:

// Bench start
Benchmark::start('heavy_data_mining');
// Some code
$data = $this->db->select()->get();
// Bench stop
Benchmark::stop('heavy_data_mining');

Benchmark::start('abnormal_loop');
// Code goes here
Benchmark::stop('abnormal_loop');

#2
Updated by Levi Stanley almost 4 years ago

Miodrag Tokić wrote:

As I understand benchmark class, you should put stop after start method:

[...]

Yes, the normal use for a benchmark class would be used in that manner. However, programmers aren't perfect, and what if they do start another benchmark key inside of another, there needs to be some indication that it happened correct? You can either throw an exception, or give back an indication that this occured. This is why I believe the designer added a count and, looped and tallied, from the beginning of time (talking about this piece of code: for ($i = 0; $i < count(self::$marks[$name]); $i++){ $time += self::$marks[$name][$i]['stop'] - self::$marks[$name][$i]['start']; $memory += self::$marks[$name][$i]['memory_stop'] - self::$marks[$name][$i]['memory_start'];}), so he would get negatives, and the count would reflect how many times the Benchmark keys was called. Anyway, just want the documentation to be updated, and the code to be optimised, that is all.

#3
Updated by Jeremy Bush almost 4 years ago

  • Priority changed from Normal to Low
  • Target version changed from 2.3.4 to 2.4

#4
Updated by Jeremy Bush almost 4 years ago

  • Status changed from New to Review

#5
Updated by Jeremy Bush over 3 years ago

  • Assignee set to Isaiah DeRose-Wilson

#6
Updated by Isaiah DeRose-Wilson over 3 years ago

  • Status changed from Review to Closed
  • % Done changed from 0 to 100

Applied in changeset r4541.

#7
Updated by Isaiah DeRose-Wilson over 3 years ago

  • Resolution set to fixed

Also available in: Atom PDF