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- Aug 3, 2017
@parkr parkr force-pushed the frozen-string-literal branch from ba4e048 to e788ad5 Aug 3, 2017
Copy link
Member

@pathawks pathawks left a comment

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

@pathawks pathawks Aug 4, 2017

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

@pathawks pathawks Aug 4, 2017

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

@pathawks pathawks Aug 4, 2017

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 parkr force-pushed the frozen-string-literal branch from e788ad5 to 4d1659c Aug 4, 2017
@parkr
Copy link
Member Author

@parkr parkr commented Aug 4, 2017

Updated, thanks for the review @pathawks!

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit 7cf5f51 into master Aug 4, 2017
1 of 2 checks passed
@jekyllbot jekyllbot deleted the frozen-string-literal branch 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants