-
Notifications
You must be signed in to change notification settings - Fork 66
Example benchmark #96
base: master
Are you sure you want to change the base?
Conversation
@@ -29,7 +29,8 @@ | |||
"hoa/visitor" : "~2.0" | |||
}, | |||
"require-dev": { | |||
"hoa/test": "~2.0" | |||
"hoa/test": "~2.0", | |||
"phpbench/phpbench": "^0.12.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be moved to hoa/test
I suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree. And we should also introduce a thin layer between PHPBench and Hoa, like we did for atoum, as Hoa\Test\Benchmark\Suite
and this class will extend it. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense (you could define f.e. setUp
etc there too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent start, thank you! This is good basis for a discussion.
* @Iterations(10) | ||
* @OutputTimeUnit("milliseconds") | ||
*/ | ||
class RulerBench |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can rename RulerBench
to Ruler
since this is an Benchmark
namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Historically this was a mandatory suffix (like Test
in PHPUnit), but not since 0.12 so, can do.
* Executes the method once (warmup) before measuring time. | ||
* | ||
* @Warmup(1) | ||
* @BeforeMethods({"setUpAssert"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you have a default setUp
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can have an abstract class which defines a setUp
method (and its annotation)
* @BeforeMethods({"setUpAssert"}) | ||
* @ParamProviders({"provideRules"}) | ||
*/ | ||
public function benchAssert($params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename the method, like case_assert
? How do you detect a method representing a benchmark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can, but then you need to use the @Bench
annotation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there is no way to say: “All methods matching this pattern are of kind bench
”?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds pretty cool!
@@ -29,7 +29,8 @@ | |||
"hoa/visitor" : "~2.0" | |||
}, | |||
"require-dev": { | |||
"hoa/test": "~2.0" | |||
"hoa/test": "~2.0", | |||
"phpbench/phpbench": "^0.12.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree. And we should also introduce a thin layer between PHPBench and Hoa, like we did for atoum, as Hoa\Test\Benchmark\Suite
and this class will extend it. Thoughts?
@@ -0,0 +1,11 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this file in a single place, like in Hoa\Test
, it will avoid to have it inside each library?
Note that we will probably create the hoa test:bench
or update the hoa test:run
command, and thus I suppose we could link this phpbench.json
file with an argument. We already do this with hoa test:run
where we already compute the atoum
command (https://github.com/hoaproject/Test/blob/f80f083a0d1cc7a044fdbca10ea46cdcdbef4c38/Bin/Run.php#L291-L326) with all the bootstrap file, configuration file etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue would be if you wanted to define f.e. a custom report.
However, most, if not all, of the configuration options can be specified at the CLI, so if you wrap it in hoa test:bench
then we can ommit the phpbench.json
file and include it only when the library has specific requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also add a Hoa phpbench extension which programatically adds reports if required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, sounds interesting. Can you tell me more about it please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Extension
instances for PHPBench are like modules for the DI container. It should be possible to add new configurations and overwrite existing ones actually this is disallowed currently.
The extensions can be enabled via. the command line or in phpbench.json
as with the example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you like to give it a try? Please, feel free to say no!
Not currently, sounds like something that could easily be configured On Mon, Sep 26, 2016 at 05:20:18AM -0700, Ivan Enderlin wrote:
|
It could be a good idea and a good feature! |
@dantleech Am I a despot if I ask you to update this PR based on PHPBench 0.12.2? |
@Hywan no problem, updated :) |
Just an example benchmark using PHPBench to get things started.