Skip to content

Added enhanced test-memory.php, originally from react/react - #59

Merged
WyriHaximus merged 4 commits into
reactphp:masterfrom
WyriHaximus:patch-3
Jul 26, 2017
Merged

Added enhanced test-memory.php, originally from react/react#59
WyriHaximus merged 4 commits into
reactphp:masterfrom
WyriHaximus:patch-3

Conversation

@WyriHaximus

@WyriHaximusWyriHaximus commented Nov 13, 2016

Copy link
Copy Markdown
Member

@clue

clue commented Nov 14, 2016

Copy link
Copy Markdown
Member

Thanks, I think it makes sense to get this in 👍

The command arguments appear rather arbitrary to me, so I guess it may make sense to use options similar to reactphp/stream#41? We may also want to reconsider our defaults here.

How about this?

php examples/benchmark-periodic-timers.php -t 5 -l StreamSelect

Also, while I understand why this is done, should we really default to adding a report every 2s?

@WyriHaximus

WyriHaximus commented Nov 14, 2016

Copy link
Copy Markdown
MemberAuthor

@clue done, that is a great suggestion. Much cleaner this way 👍 . I've also made the report interval configurable

When this is in I also want to take some examples from https://github.com/WyriHaximus/ReactBlogSeriesExamples and PR those as examples to this and other applicable repositories

jsor
jsor approved these changes Feb 4, 2017
@jsor

jsor commented Feb 8, 2017

Copy link
Copy Markdown
Member

What's the status here? Ready to merge?

@WyriHaximus

Copy link
Copy Markdown
MemberAuthor

FYI renamed the file to something better matching the currently included benchmarks

@WyriHaximus
WyriHaximus merged commit 2ae058d into reactphp:masterJul 26, 2017
@WyriHaximusWyriHaximus added this to the v0.5.0 milestone Jul 26, 2017
$t = isset($args['t']) ? (int)$args['t'] : 0;
$loop = isset($args['l']) && class_exists('React\EventLoop\\' . $args['l'] . 'Loop') ? 'React\EventLoop\\' . $args['l'] . 'Loop' : Factory::create();

if (!($loop instanceof LoopInterface)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why that instead of just adding the new above?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@WyriHaximus@clue@jsor@kelunik