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

frozen_string_literal roadmap #173

Closed
janklimo opened this issue Mar 15, 2018 · 4 comments
Closed

frozen_string_literal roadmap #173

janklimo opened this issue Mar 15, 2018 · 4 comments

Comments

@janklimo
Copy link
Contributor

Ryan, thank you for your work on this gem!

I'd like to ask if frozen_string_literal support is on the roadmap? I just ran memory profiler on one of our scripts and saw this:

screen shot 2018-03-15 at 6 18 36 pm

Some more details:
screen shot 2018-03-15 at 6 21 24 pm

I haven't looked into this much and I'm not perfectly sure if frozen_string_literal can help in our case, just wanted to open a conversation to see what your thoughts on it are. Thanks!

@rgrove
Copy link
Owner

rgrove commented Mar 15, 2018

I'm definitely open to it. I haven't had much free time lately though, so I can't promise I'll be able to get to it soon myself. I'd welcome a PR!

@flavorjones
Copy link
Contributor

See PR at #174. Hope it helps!

@rgrove
Copy link
Owner

rgrove commented Mar 15, 2018

Awesome! Thanks Mike. 😄

Merged and released 4.6.1. @janklimo, I'd be interested to hear if running with frozen string literals enabled solves your memory usage issues.

@janklimo
Copy link
Contributor Author

janklimo commented Mar 17, 2018

@rgrove I looked into this and I'm afraid this won't help with the way strings are allocated in my example above. The line in question is this one:

:node_name => node.name.downcase,

I'm not sure if there is a way to optimize the way this keeps allocating lots of identical strings. But those are not string literals, so enabling frozen_string_literal makes no difference.

However, it's great to make sanitize frozen_string_literal-friendly. Thank you @flavorjones lightning-fast response time 🚀👍 Closing.

EDIT: I have an optimization in mind and the results are promising so far. I hope to have a PR ready tomorrow.

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

No branches or pull requests

3 participants