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

For regular strings avoid igbinary serialization overhead #1786

Open
wants to merge 34 commits into
base: develop
Choose a base branch
from

Conversation

iliaal
Copy link
Contributor

@iliaal iliaal commented Jun 13, 2020

This simple change ensures that string data is stored as is without introducing overhead in speed/memory involved in serialization/deserialization steps.

In benchmarks this shows 1-2% speed improvement in terms of speed, but more critically for shorter strings it eliminates added network overhead resulting from igbinary header.

@yatsukhnenko
Copy link
Member

I don't like changes like this. It adds strange unexpected behaviour in exchange for 1-2% performance. Users always may disable serialization via setOption before sending any data not only strings and all controll and responsibility are on their side.

@iliaal
Copy link
Contributor Author

iliaal commented Jun 14, 2020

I don't like changes like this. It adds strange unexpected behaviour in exchange for 1-2% performance. Users always may disable serialization via setOption before sending any data not only strings and all controll and responsibility are on their side.

I appreciate the feedback, I made it into configuration option specifically so that it can be controlled by the user and turned off/on and is disabled by default. This makes it easier then sprinkling code with setOption() to change value and restore it back, when using a mixed environment where complex values (Arrays & Objects) should be serialized and strings left as they are.

The change from logic perspective as far as I can tell is very low risk and adds minimal (if any) cognitive overhead to internal extension code logic.

I guess another approach would be to do something like a smart_igbinary serializer that works like igbinary but implements different logic for scalar values and is distinct serialization logic within code flow. If that would be more agreeable I am happy to adjust the patch.

@yatsukhnenko
Copy link
Member

IMO there should be a division of responsibility. For phpredis all serializers look like a black box that takes a value at the input and gives a string at the output. How it works and how it can be optimized needs to be changed at the level of the serializer code. Something like smart_igbinary may be used but it should be external library (because I don't want to answer the questions about its behavior 😄) May be @michael-grunder thinks different

iliaal and others added 26 commits June 17, 2020 21:48
Presently PhpRedis will segfault if users attempt to clone either the
Redis or RedisCluster objects.

We plan on investigating proper clone support in future releases, but
for now just diable the handlers.
Without this, performing a HMGET call fails to decompress the data before
returning it to php.
ZSTD uses two defined error numbers to inform the caller when the
compressed data is invalid (e.g. wrong magic number) or the size is
unknown.

We should always know the size so abort if ZSTD returns either to us.

Fixes: phpredis#1936
Rework the session locking unit tests to be less reliant on arbitrary
sleep calls which can be very troublesome when running in Travis and
especially when running in Travis under valgrind.
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.

None yet

6 participants