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

Encode small hashes with a ziplist #285

Merged
merged 5 commits into from Mar 10, 2012

Conversation

Projects
None yet
2 participants
@pietern
Contributor

pietern commented Jan 3, 2012

This patch switches the encoding of small hashes from zipmap to ziplist (continuation of issue #188). This is a first stab, and it may need more work: tests pass, but I haven't done performance testing (throughput, memory). Because the vanilla ziplist API is used, which doesn't provide a function to replace an element, HSET is implemented as a delete followed by a push. This causes the fields in the ziplist encoded hash to be ordered by modification time (I see this more as a by-effect than a feature).

I haven't tested the loading of a zipmap encoded hash from an older RDB yet. We may want to add an integration test for that (i.e. one that includes a prefabricated RDB with such a key).

pietern added some commits Jan 2, 2012

Implements ziplistFind
To improve the performance of the ziplist implementation, some
functions have been converted to macros to avoid unnecessary stack
movement and duplicate variable assignments.
@pietern

This comment has been minimized.

Show comment
Hide comment
@pietern

pietern Jan 4, 2012

Contributor

The following graph illustrates the performance impact of the ziplist backed hashes. All commands are executed against a hash that already contains N field/value pairs (23 + 5 bytes), and use randomized field names with a field space of N. The only command that doesn't show improvement is HGET/HMGET. The impact is caused by iterating over a slightly more complex length encoding in ziplists. Because these commands need to scan the hash N times, the impact becomes more obvious with larger N.

Contributor

pietern commented Jan 4, 2012

The following graph illustrates the performance impact of the ziplist backed hashes. All commands are executed against a hash that already contains N field/value pairs (23 + 5 bytes), and use randomized field names with a field space of N. The only command that doesn't show improvement is HGET/HMGET. The impact is caused by iterating over a slightly more complex length encoding in ziplists. Because these commands need to scan the hash N times, the impact becomes more obvious with larger N.

@pietern

This comment has been minimized.

Show comment
Hide comment
@pietern

pietern Jan 4, 2012

Contributor

The memory impact for small fields/values is 9+N bytes with N being the number of pairs in the hash. The constant number is caused by the extra bytes to store length, size in bytes and a pointer to the tail element. The variable number is caused by a backlink for the value entry, which is not present in the zipmap encoding.

Contributor

pietern commented Jan 4, 2012

The memory impact for small fields/values is 9+N bytes with N being the number of pairs in the hash. The constant number is caused by the extra bytes to store length, size in bytes and a pointer to the tail element. The variable number is caused by a backlink for the value entry, which is not present in the zipmap encoding.

@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Jan 4, 2012

Owner

Thank you Pieter! Great work. I'll merge and add the missing integration test in the next days. This was an important part of the 2.6 release... very cool to have it now that we I'm going to call the feature freeze.

Owner

antirez commented Jan 4, 2012

Thank you Pieter! Great work. I'll merge and add the missing integration test in the next days. This was an important part of the 2.6 release... very cool to have it now that we I'm going to call the feature freeze.

@antirez

This comment has been minimized.

Show comment
Hide comment
@antirez

antirez Jan 4, 2012

Owner

p.s did you generated the graphs with INFO commandstats? Thanks.

Owner

antirez commented Jan 4, 2012

p.s did you generated the graphs with INFO commandstats? Thanks.

@pietern

This comment has been minimized.

Show comment
Hide comment
@pietern

pietern Jan 4, 2012

Contributor

I'll push to this pull request when I get around to the integration test. The graph uses the usec_per_call field, yes. In creating it I found that it is very helpful to be able to run a piece of code (e.g. a benchmark) against N different versions, so I'll probably add that soonish as well ;-)

Contributor

pietern commented Jan 4, 2012

I'll push to this pull request when I get around to the integration test. The graph uses the usec_per_call field, yes. In creating it I found that it is very helpful to be able to run a piece of code (e.g. a benchmark) against N different versions, so I'll probably add that soonish as well ;-)

@pietern

This comment has been minimized.

Show comment
Hide comment
@pietern

pietern Jan 25, 2012

Contributor

@antirez The integration test revealed an error in the conversion code, so it's a good thing the merge was postponed.

This patch touches a little bit of ziplist, not very invasively, but could therefore use a little more testing IMO.

Contributor

pietern commented Jan 25, 2012

@antirez The integration test revealed an error in the conversion code, so it's a good thing the merge was postponed.

This patch touches a little bit of ziplist, not very invasively, but could therefore use a little more testing IMO.

@antirez antirez merged commit d3ea4c8 into antirez:unstable Mar 10, 2012

catwell added a commit to catwell/redis that referenced this pull request Jan 28, 2013

antirez added a commit that referenced this pull request Jan 31, 2013

Merge pull request #914 from catwell/unstable
fix comments forgotten in #285 (zipmap -> ziplist)

antirez added a commit that referenced this pull request Feb 11, 2013

antirez added a commit that referenced this pull request Feb 11, 2013

Hailei pushed a commit to Hailei/redis that referenced this pull request Aug 29, 2014

Merge pull request antirez#285 from tom-martin/patch-1
Fix NPE in BuilderFactory for Double.

JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Aug 29, 2016

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