Add string_fastpath option to save on serializing simple strings#1041
Merged
Conversation
4 tasks
Collaborator
|
from some discussion we think this looks good and we will likely go this way vs trying to have the client taking raw true to handle this sort of bypass. We will likely pull it into our internal fork first and can roll it out and verify it there. |
nickamorim
approved these changes
May 13, 2025
nickamorim
left a comment
nickamorim
left a comment
Collaborator
There was a problem hiding this comment.
LGTM - thanks @byroot
Going through `Marshal.load/dump` (or other serializers) when the value is already a string is quite costly for not a whole lot of value. The only real benefit is that it automatically preserve the string encoding, but we can assume the overwhelming majority of strings are either UTF-8 or BINARY, so we can encode that in the bitflags. For strings that have a different encoding, or objects that inherit from `String` we can continue to use the serializer. Note that only the `#store` path checks for the option. `#retrieve` always handle this new format. This is so that you can seemlessly upgrade without having to flush your caches. You can first upgrade dalli and then later turn on the option. It would be nice to make this option a default at some point though.
Contributor
Author
|
I simplified the code further. Rubocop is still complaining that the method is 12 lines long instead of 10, but I refuse to comply. |
Collaborator
|
yeah don't worry about the rubocop warning we want to loosen up the rules for this gem as they have been frustrating while refactoring and making any significant changes. |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Going through
Marshal.load/dump(or other serializers) when the value is already a string is quite costly for not a whole lot of value.The only real benefit is that it automatically preserve the string encoding, but we can assume the overwhelming majority of strings are either UTF-8 or BINARY, so we can encode that in the bitflags.
For strings that have a different encoding, or objects that inherit from
Stringwe can continue to use the serializer.Note that only the
#storepath checks for the option.#retrievealways handle this new format. This is so that you can seemlessly upgrade without having to flush your caches. You can first upgrade dalli and then later turn on the option.It would be nice to make this option a default at some point though.
@danmayer @nickamorim (Ref: rails/rails#54996)