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

Lazy socket-binding and pipelining, for better predis support #27

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

stokes3452
Copy link

Adds the ability to create a phpiredis connection from an existing socket.
Also adds the ability to append commands to the redisContext (which will be flushed on demand before eventual reads), and read them back one at a time. When these are dropped into Predis's PhpiredisStreamConnection, performance increases dramatically.

@nrk
Copy link
Owner

nrk commented Aug 25, 2014

Hi @stokes3452,

Just give me a a couple of days and I'll be sure to get back to you, I'm currently a bit busy but I'm interested in these changes!

@nrk nrk mentioned this pull request Sep 3, 2014
@nrk
Copy link
Owner

nrk commented Sep 5, 2014

I finally have some time for phpiredis, sorry for the wait! I have only a few notes for now:

  1. How about renaming phpiredis_create_from_stream() to phpiredis_import_stream() along the lines of PHP's function socket_import_stream?
  2. Instead of passing a reader resource to phpiredis_create_from_stream() just to be able to set custom status and error handlers, wouldn't it be better to instantiate a reader resource internally and associate it to the connection in both s_create_connection and phpiredis_create_from_stream() and add a new phpiredis_get_reader() function that returns the underlying reader resource? This would make it possible to add custom handlers even when using the usual phpiredis_connect() function. Thoughts?
  3. Can you update tests/027.phpt to follow the new structure of the test suite?

@nrk
Copy link
Owner

nrk commented Sep 5, 2014

About the 2nd point I think I understand now why you decided to go with an optional $reader argument for phpiredis_create_from_stream(): in Predis we create and set up a phpiredis_reader resource without having a valid PHP stream which is created at a later stage due to the lazy behavior of connection objects.

I still prefer my proposal (avoiding the optional $reader argument in phpiredis_create_from_stream()), as for Predis we can simply modify Predis\Connection\PhpiredisStreamConnection to do the following:

protected function createResource()
{
    $stream = parent::createResource();

    $phpiredis = phpiredis_create_from_stream($stream);

    $reader = phpiredis_get_reader($phpiredis);
    phpiredis_reader_set_status_handler($reader, $this->getStatusHandler());
    phpiredis_reader_set_error_handler($reader, $this->getErrorHandler());

    return $phpiredis;
}

Now I wonder if it could be useful to have a phpiredis_set_reader() function just to make the interface more complete, after all we have the ability to create a reader resource in userland code by using phpiredis_reader_create().

@nrk
Copy link
Owner

nrk commented Sep 6, 2014

I just noticed that redisConnectFd() has been added only recently in hiredis but there's no tagged release including it which is a problem since I was thinking of importing these changes to phpiredis 1.0.0, I guess this makes it not possible which is a pity. Worth having it anyway after tagging phpiredis, so we can ship it in a future release.

@nrk
Copy link
Owner

nrk commented Oct 19, 2014

Ping @stokes3452?

@stokes3452
Copy link
Author

Sorry, got busy and completely lost track of this. Commit incoming, with function rename and a set_reader function instead of the optional $reader parameter. I went with an optional set_reader call over an optional get_reader call, so that people who don't want to have a reader associated with their context don't have to.

What's the new structure in the test suite?

@nrk
Copy link
Owner

nrk commented Nov 17, 2014

Don't worry @stokes3452 I just pinged you to keep the PR alive, I too got suddenly busy again so I couldn't reply sooner :-)

The structure and layout of the test suite has been recently changed in the master branch to make it a bit more tidy, you should probably merge these changes back into your branch but I can do it myself when merging locally.

On a related note, I've informally asked if there were plans to tag a new release of hiredis given the good amount of changes since the last release (and one of those changes is the very redisConnectFd() function) but unfortunately I didn't get any response yet. I'd really love to get your PR into a stable 1.0 release of phpiredis because it's a great addition and Predis would benefit from it, this time I'll try asking again by opening an issue on the hiredis repository.

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