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

Replace string literals with symbols or frozen strings #37

Merged
merged 6 commits into from
Oct 12, 2016

Conversation

janester
Copy link
Contributor

@janester janester commented Oct 6, 2016

Context

Ruby strings take up new spots of memory every time they are used. Generally it's not a problem but the memory adds up at scale. Symbols and frozen strings will always occupy the same spot in memory, which reduces the memory load and increases performance.

This Change

  • Convert some of the strings to symbols. I did this in places that it made sense for them to be symbols anyway (e.g. the strings that were getting to_sym-ed)
  • Freeze the strings that will not be changing and are used multiple times per call
  • Convert the array of arrays to hashes and use named arguments for the key and value when iterating over the hashes.

@jnunemaker @aroben @vmg can you take a look at this and let me know what you think/if I'm missing something? 🙏 💖

Copy link
Contributor

@jnunemaker jnunemaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for tackling this. Curious to see feedback from anyone at DataDog.

@jnunemaker
Copy link
Contributor

fyi @pengwynn who is probably curious about this work

@vmg
Copy link

vmg commented Oct 10, 2016

It's a good start @janester! I'm afraid that saving these allocations won't do much with the amount of gsub and string duplication there's currently in the code. I think the next step would be to switch over to bang methods and incremental string composition, like @aroben did for our fork of statsd. :)

@degemer degemer added this to the 2.1.0 milestone Oct 12, 2016
@degemer
Copy link
Member

degemer commented Oct 12, 2016

Thank you for the improvement @janester !
LGTM, will be part of the next release (~ in the next 2 weeks).

@degemer degemer merged commit e92e663 into DataDog:master Oct 12, 2016
@janester
Copy link
Contributor Author

Thanks for the review @vmg! 💖

I think the next step would be to switch over to bang methods and incremental string composition, like @aroben did for our fork of statsd. :)

👍 I'll make a new PR with those changes 😊

LGTM, will be part of the next release (~ in the next 2 weeks).

Thanks @degemer! 👍 🎉

@janester janester deleted the use-more-symbols branch October 19, 2016 20:30
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.

4 participants