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

Enforce Style/FrozenStringLiteralComment #6265

Merged
merged 1 commit into from Aug 4, 2017
Merged

Conversation

parkr
Copy link
Member

@parkr parkr commented Aug 3, 2017

The frozen_string_literal: true magic comment in Ruby can help
dramatically decrease memory allocations for new strings and can thusly
speed up your program. The intent here is for Jekyll to use less memory
and make fewer memory allocations (which must later be GC'd).

Using the output of GC.stats from script/stackprof:

Before:

total_allocated_pages: 67284
total_allocated_objects: 27434203
total_freed_objects: 23567

After:

total_allocated_pages: 63008
total_allocated_objects: 25690804
total_freed_objects: 22907

With this patch, we allocate 4,276 fewer pages, and 1,743,399 fewer objects. Holy smokes! That's huge savings.

/cc @jekyll/stability

@jekyllbot jekyllbot assigned parkr and ghost Aug 3, 2017
@parkr parkr requested review from pathawks and mattr- August 3, 2017 23:31
Copy link
Member

@pathawks pathawks left a comment

Choose a reason for hiding this comment

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

A couple comments, but I'm totally on board with this change 👍

@@ -27,7 +27,7 @@ note: This file is autogenerated. Edit /History.markdown instead.
- add plugins for multiple page pagination ([#6055]({{ site.repository }}/issues/6055))
- Update minimum Ruby version in installation.md ([#6164]({{ site.repository }}/issues/6164))
- [docs] Add information about finding a collection in `site.collections` ([#6165]({{ site.repository }}/issues/6165))
- Add {%raw%} to Liquid example on site ([#6179]({{ site.repository }}/issues/6179))
- Add {% raw %}`{% raw %}`{% endraw %} to Liquid example on site ([#6179]({{ site.repository }}/issues/6179))
Copy link
Member

Choose a reason for hiding this comment

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

This probably doesn't belong in this PR
?

exe/jekyll Outdated
@@ -1,4 +1,6 @@
#!/usr/bin/env ruby
# frozen_string_literal: true
#
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be an empty line

@@ -744,7 +759,7 @@ text/x-fortran f for
text/x-handlebars-template hbs
text/x-java-source java
text/x-lua lua
text/x-markdown markdown md mkd
text/x-markdown mkd
Copy link
Member

Choose a reason for hiding this comment

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

Mime type stuff probably doesn't belong in this PR
?

The frozen_string_literal: true magic comment in Ruby can help
dramatically decrease memory allocations for new strings and can thusly
speed up your program. The intent here is for Jekyll to use less memory
and make fewer memory allocations (which must later be GC'd).
@parkr
Copy link
Member Author

parkr commented Aug 4, 2017

Updated, thanks for the review @pathawks!

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit 7cf5f51 into master Aug 4, 2017
@jekyllbot jekyllbot deleted the frozen-string-literal branch August 4, 2017 01:27
jekyllbot added a commit that referenced this pull request Aug 4, 2017
@jekyll jekyll locked and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants