elegant icon indicating copy to clipboard operation
elegant copied to clipboard

Double loop in template causes performance degradation and *thousands* of `slugify` calls

Open mlissner opened this issue 9 years ago • 13 comments

I use Elegant for my blog and I have about 300 posts. With this many posts, generating my content was getting slow, so I took a moment to do some profiling. Turns out that when I generate my site, slugify is called about 300,000 times!

The reason, as identified by one of the Pelican contributors is a double for loop in Elegant.

According to the Pelican contributor, it looks like this could be made much faster by changing the code from:

if aTag == tag

to:

if aTag.slug == tag.slug

Seems like a simple fix that shouldn't produce any unintended consequences.

mlissner avatar Jun 22 '15 17:06 mlissner

I've tried manually applyng the change on my deployment:

diff --git a/themes/elegant/templates/article.html b/themes/elegant/templates/article.html
index 7b63b8eb..146e2f84 100644
--- a/themes/elegant/templates/article.html
+++ b/themes/elegant/templates/article.html
@@ -95,7 +95,7 @@
             <ul class="list-of-tags tags-in-article">
                 {% for tag in article.tags|sort %}
                 <li><a href="{{ SITEURL }}/{{ TAGS_URL }}#{{ tag.slug }}-ref">{{ tag }}
-                    {% for aTag, tagged_articles in tags if aTag == tag %}
+                    {% for aTag, tagged_articles in tags if aTag.slug == tag.slug %}
                     <span>{{ tagged_articles|count }}</span>
                     {% endfor %}</a></li>
                 {% endfor %}

But in my case the total generation time didn't varied (I don't have 300 posts as you)

If this is still relevant should be an easy merge

iranzo avatar Dec 03 '18 21:12 iranzo

I have 300+ posts and my server is a little ARM board, so ideal for testing bottlenecks ;)

Happy to test it out :)

silverhook avatar Dec 05 '18 09:12 silverhook

The results are in!

Before the change:

Done: Processed 367 articles, 6 drafts, 2 pages and 2 hidden pages in 368.62 seconds.

After the change:

Done: Processed 367 articles, 6 drafts, 2 pages and 2 hidden pages in 360.82 seconds.

At least in my case, it doesn’t seem to change much. Does anyone have a different experience with it?

Perhaps we just need to implement more from https://github.com/getpelican/pelican/issues/1493

silverhook avatar Dec 05 '18 09:12 silverhook

At least it doesn't break anything, so it might make sense to implement something like this ahead of the commits they mention on pelican

iranzo avatar Dec 05 '18 09:12 iranzo

@mlissner, did you try implementing the other stuff that they mention in getpelican/pelican#1493 ?

silverhook avatar Dec 06 '18 08:12 silverhook

Um, I tried everything I said I tried in that ticket, yeah? But that was four years ago.

mlissner avatar Dec 09 '18 16:12 mlissner

Perhaps pelican has been optimized, but still should be relevant to use the new approach even if there's some seconds in difference

iranzo avatar Dec 09 '18 22:12 iranzo

To the best of my knowledge, the nested loop is to be blamed for the performance degradation. The nested loop has a complexity of O(N^2).

Compare the code in articles page with the one on tags page.

Loops are used to calculate number of articles that have a particular tag. What we can do is

  1. Submit patch to pelican that adds a count property to the tag
  2. Create a pelican-plugin, that does the counting for you, which later theme can use

Existing props of tag object can be viewed here.

Then use it inside theme to show the count. So this solution to this issue can be broken down into two parts.

  1. Submit patch, and get it approved, to either pelican or pelican-plugins
  2. Change code in theme

talha131 avatar Dec 10 '18 03:12 talha131

I really think optimisations are a great thing to have and we should implement that.

But IMHO this should not be a requirement for 2.0. If we manage to get it done before everything else is done for 2.0, awesome! If we’ll still need to work for it, it should just get into the next version.

silverhook avatar Dec 10 '18 07:12 silverhook

Right. I have moved it to milestone 3.0.

talha131 avatar Dec 10 '18 07:12 talha131

Any progress here?

silverhook avatar Sep 21 '21 12:09 silverhook

Any progress here?

Not that I'm aware, if you think that this can be fixed, feel free to submit a PR so that we can get it merged ASAP in next branch

I've been lately merging some of the ones that we had there and pinging submitters for both issues and pr's

iranzo avatar Sep 21 '21 12:09 iranzo

Fixing something like this is waaaay out of my league. I merely volunteered to test it, since I’m running Pelican on an ARM board, so any performance changes show really well there.

silverhook avatar Sep 21 '21 13:09 silverhook