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

Imported global functions #167

Merged
merged 4 commits into from
Jun 28, 2018

Conversation

Ocramius
Copy link
Contributor

This keeps me awake for now. Silly patch, still kinda relevant for this low-level component

Note that I wanted to use doctrine/coding-standard on it, but the package seems to still support PHP 5.3 (!!!!!!!!!!!!!!!!!!).

@Ocramius
Copy link
Contributor Author

Ran the benchmars - this makes ~10% impact - wasn't expecting that O_o

@WyriHaximus
Copy link
Member

Ran the benchmars - this makes ~10% impact - wasn't expecting that O_o

Ow it could get even faster if we would jump to PHP 7.0+ (which I personally would love to see in event loop 1.1 (hint hint @clue @jsor)). And use $listener($stream); instead of \call_user_func($listener, $stream);

but the package seems to still support PHP 5.3 (!!!!!!!!!!!!!!!!!!).

Yeah that's a thing :(. As a said 1.1?

@Ocramius
Copy link
Contributor Author

Ow it could get even faster if we would jump to PHP 7.0+ (which I personally would love to see in event loop 1.1 (hint hint @clue @jsor)). And use $listener($stream); instead of \call_user_func($listener, $stream);

No difference there: the opcode is the same one.

@Ocramius
Copy link
Contributor Author

Ah, interesting, tests fail because of ancient PHPUnit versions.

Will have to use a real timer instead of a mock.

@Ocramius Ocramius force-pushed the feature/imported-global-functions branch from d83c940 to b048bd7 Compare June 13, 2018 13:04
@Ocramius Ocramius force-pushed the feature/imported-global-functions branch from 5fded13 to 88d35a6 Compare June 14, 2018 01:50
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@Ocramius Thank you very much for filing this PR, the changes LGTM and I can confirm some noticeable performance improvements here 👍 🎉

It looks like this PR currently addresses two things, may I ask you to squash this into two commits? :shipit:

$timers = new Timers();

$timer1 = new Timer(0.1, function () {});
$timer2 = new Timer(0.1, function () {});
Copy link
Member

@clue clue Jun 19, 2018

Choose a reason for hiding this comment

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

LGTM as-is, but for the reference and because I'm sure you're going to appreciate this: Legacy PHPUnit does not support $this->createMock('React\EventLoop\TimerInterface'), but this can easily be worked around with $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock(). 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I avoided the mock builder and went with the real instance. Would've used an anonymous class if this was 7.0+

@clue clue added this to the v0.5.3 milestone Jun 19, 2018
Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

LGTM, but can you as requested by @clue squash it in two logical commits?

@Ocramius
Copy link
Contributor Author

squash it in two logical commits?

There are 4 atomic commits that are not supposed to be squashed

@WyriHaximus
Copy link
Member

squash it in two logical commits?

There are 4 atomic commits that are not supposed to be squashed

Makes sense 👍 :shipit:

@clue
Copy link
Member

clue commented Jun 28, 2018

squash it in two logical commits?

There are 4 atomic commits that are not supposed to be squashed

Debatable - but I don't really like the idea of discussing this, given this this PR includes some very real improvements, thank you! :shipit: 👍

@jsor jsor merged commit fc1d78e into reactphp:master Jun 28, 2018
@Ocramius Ocramius deleted the feature/imported-global-functions branch June 29, 2018 10:23
@clue clue mentioned this pull request Jan 11, 2019
WyriHaximus added a commit to WyriHaximus-labs/event-loop that referenced this pull request Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants