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

#73 Adds setup for simple benchmarks #80

Merged
merged 4 commits into from
Dec 11, 2016

Conversation

jaapio
Copy link
Contributor

@jaapio jaapio commented Dec 7, 2016

These changes add a simple benchmark setup.
This allows us to create and run the benchmark tests.

Releated to #73

These changes add a simple benchmark setup.
This allows us to create and run the benchmark tests.
@jaapio
Copy link
Contributor Author

jaapio commented Dec 7, 2016

I started with this small setup. Let me know if there are features that I didn't cover

Copy link
Member

@shochdoerfer shochdoerfer left a comment

Choose a reason for hiding this comment

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

For an initial version it looks good, see my remarks for some minor changes. In addition to that I would love to see benchmarks for the different bean configuration options (singleton / non-singleton, lazy / non-lazy, alias) to see which impact the different configuration options have.

Do you have a good idea how to add the benchmarks to the README.md file? Since we have the "production tuning" section there it might be a good idea to include the benchmark results there somehow.

/**
* @Bean
*/
public function mySimpleService() : A {
Copy link
Member

Choose a reason for hiding this comment

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

For consistency reasons the method should be named A. You could define "mySimpleService" as alias. That way we could also run a bechmark trying to retrieve beans by the alias.

/**
* @Bean
*/
public function B() : B {
Copy link
Member

Choose a reason for hiding this comment

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

Please use the PSR-2 standard for formatting the configuration class.

/**
* @Configuration
*/
class MyConfiguration
Copy link
Member

Choose a reason for hiding this comment

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

Better rename the class to BenchmarkConfiguration so that it is clear what the purpose of the class is.

\bitExpert\Disco\BeanFactoryRegistry::register($this->disco);
}

public function benchCreateInstance()
Copy link
Member

Choose a reason for hiding this comment

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

For consistency reasons the method should be named benchCreateSimple()

@jaapio
Copy link
Contributor Author

jaapio commented Dec 7, 2016

phpbench supports reporting to markdown, so it will be very easy to add those results to the readme.

I will have a look at your requested changes and add more benchmarks for other cases. Glad you like the initial setup.

Fixes number of psr2 issues and clearified some class and method naming
for consistency
Report for github can be generated using the command below

`vendor/bin/phpbench run --report=aggregate -o github`

public function benchCreateSimpleAliased()
{
$this->disco->get('mySimpleService');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason this bench will run forever.

Copy link
Member

Choose a reason for hiding this comment

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

Seems to work fine for me. Which PHP version did you use to run the test?

@jaapio jaapio changed the title Adds setup for simple benchmarks #73 Adds setup for simple benchmarks Dec 8, 2016
@jaapio
Copy link
Contributor Author

jaapio commented Dec 11, 2016 via email

@shochdoerfer shochdoerfer merged commit 5f123af into bitExpert:master Dec 11, 2016
@shochdoerfer shochdoerfer added this to the 0.7.0 milestone Dec 11, 2016
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.

2 participants