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

Memoize the return value of Document#url #6266

Merged
merged 1 commit into from Aug 4, 2017
Merged

Conversation

parkr
Copy link
Member

@parkr parkr commented Aug 4, 2017

Running bundle exec rake site:generate locally, this brings generation time from 16 seconds down to 8 seconds.

done in 16.421 seconds.
done in 8.685 seconds.

@parkr parkr requested review from pathawks and a user August 4, 2017 02:16
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.

That's a huge savings! 🎉

Running 'bundle exec rake site:generate' locally, this brings generation time from 16 seconds down to 8 seconds.
@parkr
Copy link
Member Author

parkr commented Aug 4, 2017

Lol @ ruby. Also lol @ us not catching this! If you haven't checked out stackprof, it is so cool.

$ export BENCHMARK=1
$ bundle install
$ script/stackprof # measures cpu
$ script/stackprof object # measures memory

@parkr
Copy link
Member Author

parkr commented Aug 4, 2017

@jekyllbot: merge +fix

@ghost
Copy link

ghost commented Aug 4, 2017

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit d8dfc33 into master Aug 4, 2017
@jekyllbot jekyllbot deleted the memoize-document-url branch August 4, 2017 19:53
jekyllbot added a commit that referenced this pull request Aug 4, 2017
@ghost
Copy link

ghost commented Aug 4, 2017

@parkr could it be that jekyllbot doesn't accept comments that have multiple spaces in them (like yours did)?

@parkr
Copy link
Member Author

parkr commented Aug 4, 2017

Perhaps! I'm not sure. Thanks for submitting the correction for me :)

@parkr parkr mentioned this pull request Aug 12, 2017
6 tasks
parkr added a commit that referenced this pull request Aug 12, 2017
Backport #6266 for v3.5.x: Memoize the return value of Document#url
parkr added a commit that referenced this pull request Aug 12, 2017
parkr added a commit that referenced this pull request Aug 12, 2017
parkr added a commit that referenced this pull request Aug 12, 2017
Backport #6266 for v3.5.x: Memoize the return value of Document#url
@ianthehenry
Copy link

This breaks, e.g., generators that programatically calculate permalinks -- you must now explicitly un-set the url property after setting the permalink to forget the memoized value.

I feel that as it used to be a dependent property, changing any of the values it depends on should invalidate the cache. But I don't know how to do that when it depends on values stored in hashes...

@parkr
Copy link
Member Author

parkr commented Aug 17, 2017

Hey @ianthehenry! Can you give a simple example? We could set the @url value to nil if we can catch the changes. This change is important to keep, as it is a significant performance gain.

@ianthehenry
Copy link

ianthehenry commented Aug 17, 2017

Here was a plugin that I had written copied almost verbatim from StackOverflow:

# Because we put our posts in their own directories:
#
# _posts/2017-05-11-foo/index.md
#
# Instead of their own files:
#
# _posts/2017-05-11-foo.md
#
# The default slug is "foo/index". But this is annoying, so we strip it.

module Jekyll
  class PermalinkRewriter < Generator
    safe true
    priority :low

    def generate(site)
      site.posts.docs.each do |post|
        unless post.data.member?('permalink')
          post.data['permalink'] = post.data['slug'].sub(/\/index$/, '') + '/'
        end
      end
    end
  end
end

After upgrading to 3.5.2, I had to make this tweak:

module Jekyll
  class Document
    def invalidate_url
      @url = nil
    end
  end

  class PermalinkRewriter < Generator
    safe true
    priority :low

    def generate(site)
      site.posts.docs.each do |post|
        unless post.data.member?('permalink')
          post.data['permalink'] = post.data['slug'].sub(/\/index$/, '') + '/'
          # As of 3.5.2, post.url is memoized, and not recalculated when one of
          # the properties it's based on changes.
          #
          # https://github.com/jekyll/jekyll/pull/6266
          post.invalidate_url
        end
      end
    end
  end
end

Which, you know, was after a bit of head-scratching and then looking for probable commit messages.

I suppose I could also just override the url getter on a post. That feels less hacky than this, I suppose. But yeah; it was a thing that used to work that stopped working.

@jekyll jekyll locked and limited conversation to collaborators Jul 12, 2019
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

4 participants