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

Add frozen_string_literal: true magic comment to reduce string allocations in modern Rubies #883

Merged
merged 3 commits into from
Sep 21, 2018

Conversation

ashmaroli
Copy link
Contributor

Reduce memory usage to allocate duplicate strings in Ruby 2.3 and above by adding the magic comment:

# frozen_string_literal: true

@dblessing
Copy link
Collaborator

Thanks @ashmaroli. Did you add this to all files, or did you encounter any places where this broke something and omitted it on those files?

I admit, this is the first I'm learning about the magic comment. I see some information that suggests it has to be the first line of every file. Does it still work as the second line since the utf-8 comments are in most files?

@ashmaroli
Copy link
Contributor Author

The comments were added via Rubocop's --autocorrect feature. It has been added to every file under lib/ and tested against Rouge's existing Test-Suite.
The few places where Rouge expected mutable (non-frozen) strings, I replaced string literals with String.new (which are functionally equivalent).

The test failures I encountered in the intermediate steps above is proof that the magic comment works even when its on the second line. (You can also easily test that on any local Ruby script using Ruby 2.3 and above.)
On that note, the encoding: utf-8 comment is actually no longer required since UTF-8 is the default encoding for the Ruby Interpreter since Ruby 2.0. I kept it intact here since its beyond the scope of this PR.

@dblessing
Copy link
Collaborator

This seems OK to me. But I would like to get @gfx or @jneen to give the 👍 , too.

@dblessing
Copy link
Collaborator

@ashmaroli Sorry for the delay on this. Are you interested in fixing this up with the latest master? It would be nice to get going. I just did some memory profiling and I see that this causes memory allocation for the String class alone to drop from 7.4 MB to 1.4MB. 😱

@ashmaroli
Copy link
Contributor Author

Are you interested in fixing this up with the latest master?

@dblessing Yes, I'm 👍 👍 on updating this with the latest master.. I'll push a couple of commits this Sunday..

I just did some memory profiling

Would love it if the script for that was pushed into this repository..
While you're at it, if Rouge has a dedicated benchmark suite, that fine as well..
I'm planning to replace String.new calls added in this PR with String#+ to unfreeze a string because the latter is a lot faster than the former..

@dblessing
Copy link
Collaborator

Would love it if the script for that was pushed into this repository..

I plan to make it a Rake task(s) once I have something solid. I'm still playing at the moment.

Thanks for your willingness to pick this up again. Ping me when ready.

@ashmaroli
Copy link
Contributor Author

ashmaroli commented Sep 9, 2018

@dblessing I've updated the branch with master and added the pragma to all files run through the Ruby executable. I've deferred using the more performant +(str) (to unfreeze strings where necessary), till Rouge drops support for Rubies older than v2.3.0..

Awaiting your feedback.. 😃

@dblessing
Copy link
Collaborator

Thanks @ashmaroli Testing now

@dblessing
Copy link
Collaborator

Nice, albeit modest, improvements in memory usage and number of objects. I'll take it :)

  • Allocated memory by Rouge dropped by 290KB
  • Allocated memory by String class dropped by 373KB
  • Allocated objects by String class dropped by 9,500
  • Retained memory (which is the important one for memory) dropped by 198KB
  • Retained objects (which is the important one for num objects) dropped by 6,055.

@dblessing dblessing merged commit f8f2295 into rouge-ruby:master Sep 21, 2018
@ashmaroli ashmaroli deleted the frozen-strings branch September 21, 2018 18:23
@ashmaroli
Copy link
Contributor Author

ashmaroli commented Sep 21, 2018

Thank you for merging, and yes, for posting the numbers as well 😃 👍

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.

2 participants