Skip to content
This repository has been archived by the owner on Sep 20, 2021. It is now read-only.

issue 29: If the execution is < 1ms then global > 100 % #30

Closed
wants to merge 1 commit into from

Conversation

1e1
Copy link
Contributor

@1e1 1e1 commented Aug 3, 2015

fix #29

Moreover, test pause/resume abilities

<?php

require __DIR__ . DIRECTORY_SEPARATOR . 'vendor' . DIRECTORY_SEPARATOR . 'autoload.php';

use Hoa\Bench;


$tab = \SplFixedArray::fromArray(['Gg', 'est', 'le', 'plus', 'beau']);

$bench = new Bench();

$bench->one->start();
$bench->oneBis->start();

for($i=0;$i<$tab->count();$i++){

    echo $tab[$i];

}

$bench->one->stop();
$bench->oneBis->pause();


$bench->two->start();

//le foreach

foreach($tab as $value){

    echo $value;
}

$bench->two->stop();


echo $bench;


$bench->three->start();

//le foreach

foreach($tab as $value){

    echo $value;
}

$bench->three->stop();


echo $bench;
Ggestleplusbeau
__global__  ||||||||||||||||||||||||||||||||||||||||||||||||||||     0ms, 100.0%
one         |||||||||||                                              0ms,  20.7%
oneBis      |||||||||||||||||||||||||||||||||                        0ms,  63.2%
two         |||                                                      0ms,   6.6%
three       ||                                                       0ms,   4.4%

$runningMarks = [];

foreach ($this as $mark) {
if ($mark->isRunning()) {
Copy link
Member

Choose a reason for hiding this comment

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

if (true === $mark->isRunning())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mmmh, and false === $mark->isPause()
I didn't realized oneBis is running again, but it's manually paused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The result with the Pause test is:

Ggestleplusbeau
__global__  ||||||||||||||||||||||||||||||||||||||||||||||||||||     1ms, 100.0%
one         ||||                                                     0ms,   7.8%
oneBis      ||||                                                     0ms,   7.4%
two         |                                                        0ms,   2.1%
three       |                                                        0ms,   2.4%

Match the new values of oneBis with the previous ones.

@Hywan Hywan assigned thehawk970 and unassigned Hywan Aug 4, 2015
@Hywan
Copy link
Member

Hywan commented Aug 4, 2015

👍 for me after the small modifications.

@Hywan
Copy link
Member

Hywan commented Aug 5, 2015

Need to fix #30 (comment) before merging.

$runningMarks = [];

foreach ($this as $mark) {
if (true === $mark->isRunning() && false === $mark->isPause()) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe isRunning should return false if isPause returns true?

Copy link
Member

Choose a reason for hiding this comment

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

Bench/Mark.php

Line 307 in ee3df12

return $this->_running;

return $this->_isRunning && $this->isPause();

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree with you.
isRunning() is related to ->_isRunning and isPause() to ->_isPause.
This condition should be in a other method.

I purpose 2 choices:

1-

  • rename /isRunning/ to isInitialized
  • add isRunning() for initilized unpaused mark.

2-

  • add isTiming()

The second point is lighter ;)

Copy link
Member

Choose a reason for hiding this comment

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

The difference between isTiming and isRunning is not clear. isStarted would be a nicer name for isRunning… Any way… let's go for your patch and please open a new issue to address this problem.

@Hywan
Copy link
Member

Hywan commented Aug 6, 2015

Also, can you open two issues, please? One to update the English documentation and another for the French documentation.

This was referenced Aug 6, 2015
@Hywan
Copy link
Member

Hywan commented Aug 12, 2015

@1e1 You make good contributions and I really appreciate them. However, I reword your commit message by this one aeb9a65:

Introduce the pause and resume methods.

In issue 29, for really short execution, the global mark was longer
than 100% of the time, which is not possible. This is due to the fact we
take time between each mark computation when drawing the statistics.

Consequently, the pause and resume methods are introduced to
respectively pause all runnings tasks and resume a specific set of
marks.

We use these methods in the getStatistic method to pause all the
running tasks and resume them before and after actually computing the
statistics.

It much more respects the contributor guide requirement. We explain the issue and how we address this issue. Also the commit title much more reflect what the commit does.

Just after, I created a new commit to make resume a static method: ed10581.

Finally, I reordered the methods in the class (a small detail): 46d656a.

Thank you very much!

@Hywan Hywan closed this Aug 12, 2015
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

If the execution is < 1ms then global > 100 %
3 participants