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

Performance: Sort files only once #3707

Merged
merged 1 commit into from May 18, 2015

Conversation

Projects
None yet
5 participants
@fw42
Contributor

fw42 commented May 16, 2015

Fixes #3705.

Turns out the reason that it was slow for me was not that it was sorting at all but that it was sorting the same list of files over and over again.

Solution: Sort only once at the end of #read, not every time we load more files from a directory (those functions are called recursively via #read_directories). This also ensures backwards compatibility for the regular code path (unless you have a plugin that directly calls #retrieve_posts etc. or stuff like that).

@envygeeks @parkr @alfredxing

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 17, 2015

Contributor

👍

Contributor

envygeeks commented May 17, 2015

👍

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing May 18, 2015

Member

👍

What's the performance improvement you're seeing with this?

Member

alfredxing commented May 18, 2015

👍

What's the performance improvement you're seeing with this?

@parkr

View changes

Show outdated Hide outdated lib/jekyll/reader.rb Outdated
@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 May 18, 2015

Contributor

What's the performance improvement you're seeing with this?

not crazy much.. about 500ms of ~5s (for initial build and for each regeneration cycle) on a site with about 6000 files. but I guess you have to start somewhere :-)

Contributor

fw42 commented May 18, 2015

What's the performance improvement you're seeing with this?

not crazy much.. about 500ms of ~5s (for initial build and for each regeneration cycle) on a site with about 6000 files. but I guess you have to start somewhere :-)

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr
Member

parkr commented May 18, 2015

:shipit:

@parkr parkr added the Enhancement label May 18, 2015

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 18, 2015

Contributor

Pending responses it'll merge!

Contributor

envygeeks commented May 18, 2015

Pending responses it'll merge!

@envygeeks envygeeks added :shipit: labels May 18, 2015

@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 May 18, 2015

Contributor

Updated: Renamed the method

Contributor

fw42 commented May 18, 2015

Updated: Renamed the method

@envygeeks envygeeks removed :shipit: labels May 18, 2015

envygeeks added a commit that referenced this pull request May 18, 2015

@envygeeks envygeeks merged commit 06777a5 into jekyll:master May 18, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fw42

This comment has been minimized.

Show comment
Hide comment
@fw42

fw42 May 18, 2015

Contributor

Would you mind releasing a beta5 release to Rubygems?

Contributor

fw42 commented May 18, 2015

Would you mind releasing a beta5 release to Rubygems?

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 18, 2015

Contributor

It'll happen sometime in the next few days because I have a few more changes I would like to get into beta5 (aside from our new JRuby support.) Like thread pooling and possibly even collection fixes.

Contributor

envygeeks commented May 18, 2015

It'll happen sometime in the next few days because I have a few more changes I would like to get into beta5 (aside from our new JRuby support.) Like thread pooling and possibly even collection fixes.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks May 18, 2015

Contributor

If you need though I can cut a new release of our master image for Docker.

Contributor

envygeeks commented May 18, 2015

If you need though I can cut a new release of our master image for Docker.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 19, 2015

Member

Why not just call those beta6, @envygeeks?

Member

parkr commented May 19, 2015

Why not just call those beta6, @envygeeks?

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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