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

Check default front matter scope against symbols #8393

Merged
merged 3 commits into from
Oct 11, 2020

Conversation

ashmaroli
Copy link
Member

  • This is a 🔨 code refactoring.

Summary

The type attribute of Jekyll's core convertible classes are Symbols. i.e. Pages are :pages, documents are the parent collection-label symbolized, etc

(The exception being Jekyll::StaticFile and Jekyll::Layout which have type as nil).

Coercing a Symbol (or nil) to String allocates the string to memory on each call. Since this coercion in the code below is only for comparison, we can instead coerce the string values to Symbols and avoid allocating temporary strings:

def applies_type?(scope, type)
!scope.key?("type") || scope["type"].eql?(type.to_s)
end

@ashmaroli ashmaroli requested a review from a team September 16, 2020 16:59
@ashmaroli
Copy link
Member Author

ashmaroli commented Sep 17, 2020

Third Party Repo Profile Summary

--- master
+++ PR

- Total allocated: 282.43 MB (2467827 objects)
+ Total allocated: 281.58 MB (2446531 objects)
  Total retained:  37.81 MB (112069 objects)

Copy link
Member

@DirtyF DirtyF left a comment

Choose a reason for hiding this comment

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

@ashmaroli
Copy link
Member Author

I've restarted the check-run. Let's see if that makes any difference 😉

@ashmaroli
Copy link
Member Author

The new check run has different timestamps:

--- https://github.com/jekyll/jekyll/runs/1129387000?check_suite_focus=true#step:7:18
+++ https://github.com/jekyll/jekyll/runs/1129680368?check_suite_focus=true#step:7:18

-                    done in 32.912 seconds.
-                    done in 36.238 seconds.
-                    done in 34.763 seconds.
+                    done in 29.318 seconds.
+                    done in 30.575 seconds.
+                    done in 30.011 seconds.

But the memory usage remains the same.

The takeaway is that the build job is an unreliable metric 😅

@ashmaroli ashmaroli added the memory-optimization ⚡ Reduced memory usage for the work done label Sep 18, 2020
@ashmaroli ashmaroli added this to the 4.2 milestone Sep 25, 2020
@ashmaroli
Copy link
Member Author

@jekyllbot: merge +fix

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fix frozen-due-to-age memory-optimization ⚡ Reduced memory usage for the work done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants