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

Remove costly default key #1778

Merged
merged 1 commit into from Jun 9, 2017

Conversation

Projects
None yet
2 participants
@schveiguy
Contributor

schveiguy commented Jun 9, 2017

Currently when using redis for sessions, the default key is the session id, and the default value is the session id. The session id's length is 86 characters long. This means that for every session, redis will require 86 * 3 characters minimum of memory, instead of just 86 characters. The key name and data isn't important at all. It's just a minimal default key so the session can be created (redis does not allow empty hashmaps).

This replaces the very long name and value with a small one. While it may not make a huge difference (comes out to about 16.5MB difference if you have 100k sessions), it seems unnecessarily wasteful. In my case, at least, this "default" key is more than 2x larger than all the session data I am storing.

@schveiguy

This comment has been minimized.

Show comment
Hide comment
@schveiguy

schveiguy Jun 9, 2017

Contributor

I also changed hmset to hset, as you are only setting one key.

Contributor

schveiguy commented Jun 9, 2017

I also changed hmset to hset, as you are only setting one key.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Jun 9, 2017

Member

Thanks! Incidentally I stumbled over the same line a few days ago, but didn't take the time to change it. Good to have this taken care of.

Member

s-ludwig commented Jun 9, 2017

Thanks! Incidentally I stumbled over the same line a few days ago, but didn't take the time to change it. Good to have this taken care of.

@s-ludwig s-ludwig merged commit acfc3f5 into vibe-d:master Jun 9, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@schveiguy

This comment has been minimized.

Show comment
Hide comment
@schveiguy

schveiguy Jun 10, 2017

Contributor

Thanks!

Contributor

schveiguy commented Jun 10, 2017

Thanks!

@schveiguy schveiguy deleted the schveiguy:patch-2 branch Jun 10, 2017

s-ludwig added a commit that referenced this pull request Sep 2, 2017

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