Skip to content

Conversation

@clue
Copy link
Member

@clue clue commented Dec 20, 2017

I'm filing this PR as an RFC to see if it makes sense to drop these type hints.

@clue
Copy link
Member Author

clue commented Dec 20, 2017

I've seen a 5% to 10% performance improvement running any of the benchmarks from the examples/folder and have also seen some similar performance improvements in running some higher level applications (I/O intensive) with this applied.

Note that I'm not suggesting we remove all type hints. This PR merely aims to remove the callable type hint only – also for consistency reasons with the whole API. Also note that this does not contradict our efforts to eventually update to PHP 7 only (reactphp/reactphp#374), in which case we will likely reconsider adding consistent type hints for all relevant APIs.

@WyriHaximus WyriHaximus added this to the v0.5.0 milestone Dec 20, 2017
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.

That is a nice and welcome performance improvement, LGTM :shipit:

Copy link
Member

@jsor jsor left a comment

Choose a reason for hiding this comment

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

mixed $feelings, but :shipit: 😉

@jsor jsor merged commit e18ee06 into reactphp:master Dec 22, 2017
@clue clue deleted the callable branch December 22, 2017 14:57
Copy link

@morrisonlevi morrisonlevi left a comment

Choose a reason for hiding this comment

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

... what? I'm not a react-php user so definitely feel free to ignore me but... what does "consistency" even mean here? I would also hope any loss of type-safety for performance reasons would also be backed by reproducible numbers; is there some missing context from this PR?

Edit: A bit of background: I've altered the engine to no-op the type-checking in the past. This did not register a significant improvement gain; this is why I do not trust the claim. Again, I'm not using this project so take my complaint with that in mind.

@kelunik
Copy link
Contributor

kelunik commented Dec 22, 2017

I can't reproduce any improvements in examples/94-benchmark-timers-delay.php either. Which benchmark specifically did you run and which numbers did you get?

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.

5 participants