Reodering of pages takes a long time and also times out

[tdelam]

Current site has about 15 pages and 2 engines. When I order pages and click on "Done reordering pages" it sits here with the "spinner" for a good 10-12 seconds.

After I noticed this I double the pages to 30 pages and tried to reorder again and it times out completely.

[Matho]

Do you use 2.0.8 ? In < 2.0.8 version there is bug, which produces all tree reordering and of course more time. But yes, act-as-nested-set produce many sql queries when reordering tree. It should be more efficient. Try hide all children by default in config.auto_expand_admin_tree = false in initializers/refinery/pages.rb .

[tdelam]

@Matho I am using edge.

I was talking with parndt and robyurkowski and it sounds like the slugs get re-generated instead of just slotting the new ordered set into the rght lft positions so it sounds more like that would be the bottleneck.

[Matho]

I dig deeper a little and I think I have found problem. (but only partial). In crud.rb (under refinerycmscore/core/lib/refinery) in method update_positions is called #{class_name}.rebuild! , which is alias to rebuild_with_invalidate_cached_urls! . There is called find_each - it select all pages from DB and run method invalidate_cached_urls for each. And there, the problem is [self, children].flatten.each. If you have 140 pages, like I have, that produce 140 SELECT queries looking for children of each page to create url adress and flush it from disk. If the main goal of that function is to flush page cache (on disk), it is extremely inefficient. We can flush disk cache by call expire_cache from pagesweeper which flush disk folder by cache_root.rmtree .

As I see the commit message in #1706 (comment) the main problem was, that cache was not flushed after update_position, so he has problems by cached content on frontend.

But as I see - there is after filter lambda{::Refinery::Page.expire_page_caching}, :only => [:update_positions] , which should expire cache after update_positions. So only delete code #{class_name}.rebuild! if #{class_name}.respond_to?(:rebuild!) and all code associated with it, should speed up this process.

[parndt]

The problem is that if we delete the rebuild! code then nothing will get reordered, right? That's what rebuild! does :-)

I think the better way forward is to optimise so that we don't get 140 SELECT queries using efficient invalidation as @Matho says.

[Matho]

No, no, reordering with that line commented works.All the work it do is invalidate page cache very inefficiently. (Expect method rebuild_without_invalidate_cached_urls! - I'm unable to find, where it is from )

[parndt]

Looks like it was only ever merged to 2-0-stable (cc @ugisozols) so it is defined here:

https://github.com/refinery/refinerycms/blob/2-0-stable/pages/app/models/refinery/page.rb#L191-L195

alias_method_chain aliases the original method as method_name_without_alias_chain i.e. rebuild_without_invalidate_cached_urls! so this is the original rebuild! (which is defined in awesome_nested_set)

[Matho]

Ah now it makes sense, where it is from. With that commented line reordering worked. It rebuilds only if tree is not valid and in my situation it allways was valid so no rebuild was needed. But that 140 queries was produced by code find_each { |page| page.send :invalidate_cached_urls }

[parndt]

Makes sense. We need to optimise what happens when find_each is called (perhaps not calling it but just invalidating all cached urls?)

[Matho]

Yes - drop that line find_each. I think page cache is flushed - there is after filter in pages_controller. But I didn't test it, if it work, yet.

[Matho]

Tested with commented find_each. (but without running test suite - I have problems with run tests) page cache is correctly flushed after update position by calling expire_page_caching in after_filter.

[ugisozols]

Speaking about cache - is it even created? As I understand https://github.com/refinery/refinerycms/blob/2-0-stable/pages/app/models/refinery/page.rb#L339-341 should create new cache entries in case it can't find them but that method isn't called anywhere in the code.

Am I missing something?

Btw, by removing find_each specs still pass (because we don't have any spec coverage for that part ;)).

[parndt]

So what should we do here?

[ugisozols]

@parndt see ^ commit message.

[ugisozols]

#1998 was merged. Closing.