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

Conversation

Projects
None yet
5 participants
@parkr
Member

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 oe Aug 4, 2017

@pathawks

That's a huge savings! 🎉

Memoize the return value of Document#url
Running 'bundle exec rake site:generate' locally, this brings generation time from 16 seconds down to 8 seconds.
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 4, 2017

Member

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
Member

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

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 4, 2017

Member

@jekyllbot: merge +fix

Member

parkr commented Aug 4, 2017

@jekyllbot: merge +fix

@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Aug 4, 2017

Member

@jekyllbot: merge +fix

Member

oe commented Aug 4, 2017

@jekyllbot: merge +fix

@jekyllbot jekyllbot merged commit d8dfc33 into master Aug 4, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot added bug fix labels Aug 4, 2017

@jekyllbot jekyllbot deleted the memoize-document-url branch Aug 4, 2017

jekyllbot added a commit that referenced this pull request Aug 4, 2017

@oe

This comment has been minimized.

Show comment
Hide comment
@oe

oe Aug 4, 2017

Member

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

Member

oe commented Aug 4, 2017

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

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 4, 2017

Member

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

Member

parkr commented Aug 4, 2017

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

@parkr parkr referenced this pull request Aug 12, 2017

Closed

Release v3.5.2 #6292

6 of 6 tasks complete

parkr added a commit that referenced this pull request Aug 12, 2017

parkr added a commit that referenced this pull request Aug 12, 2017

Merge pull request #6300 from jekyll/3.5-stable-backport-6266
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

Merge pull request #6301 from jekyll/3.5-stable-backport-6266
Backport #6266 for v3.5.x: Memoize the return value of Document#url
@ianthehenry

This comment has been minimized.

Show comment
Hide comment
@ianthehenry

ianthehenry Aug 17, 2017

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...

ianthehenry commented Aug 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 17, 2017

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@ianthehenry

ianthehenry 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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment