Opened 9 years ago

Last modified 6 years ago

#359 reopened enhancement

Improve NAP performance

Reported by: flack Owned by: flack
Priority: blocker Milestone: 8.09.10 Ragnaroek
Component: MidCOM core Version: 8.09 Ragnaroek
Keywords: performance Cc:

Description

Currently, when building the nav tree, midcom_helper_nav loads all the components and their dependencies when constructing a node object (in _basicnav.php line 422). F.x. on the start page of my openpsa installation, nine components are loaded during the drawing of the fi.proti.navigation. In total, the start page loads 23 components, so more than 30% are caused by midcom_helper_nav.

It would be nice if a way could be found to reduce this. F.x. in the code, a cache mechanism for nap is mentioned, but it is commented out because of a bug #252, but I guess the number refers to an old bugtracker

Change History (80)

comment:2 Changed 9 years ago by flack

After some optimizations of the components, the numbers mentioned in the post are now changed to 5 components loaded directly by o.o.mypage during the display of the start page, while 11 components get loaded by midcom_helper_nav, so over 60% of the component loads are done when the NAP tree is built.

comment:3 Changed 9 years ago by bergie

(In [18472]) Remove some unnecessary queries, refs #359

comment:4 Changed 9 years ago by bergie

(In [18479]) Some tweaking of navigation performance, refs #359

comment:5 Changed 9 years ago by bergie

  • Milestone changed from 8.09.2 Ragnaroek to 8.09.3 Ragnaroek

Some work has been done in .2. More in .3

comment:6 Changed 9 years ago by bergie

  • Milestone changed from 8.09.3 Ragnaroek to 8.09.4 Ragnaroek
  • Summary changed from Make midcom_helper_nav less resource-intensive to Enable NAP cache

comment:7 Changed 8 years ago by bergie

  • Milestone changed from 8.09.4 Ragnaroek to 8.09.5 Ragnaroek

comment:8 Changed 8 years ago by bergie

  • Milestone changed from 8.09.5 Ragnaroek to 8.09.6 Ragnaroek

comment:9 Changed 8 years ago by piotras

  • Milestone changed from 8.09.6 Ragnaroek to 8.09.7 Ragnaroek

comment:10 Changed 7 years ago by flack

  • Owner changed from bergie to flack
  • Summary changed from Enable NAP cache to Improve NAP performance

Changed the title because caching is one way to make this faster, but if other ways can be found, noone would be sad either...

comment:11 Changed 7 years ago by flack

(In [26394]) remove some duplication for improved readability, loosely refs #359

comment:12 Changed 7 years ago by flack

(In [26395]) remove useless counter, refs #359

comment:13 Changed 7 years ago by flack

(In [26396]) mark private methods and attributes, refs #359

comment:14 Changed 7 years ago by flack

(In [26398]) use object cache where possible, refs #359

comment:15 Changed 7 years ago by flack

(In [26399]) use statuc node cache to reduce overhead for dynamic_loads, refs #359

comment:16 Changed 7 years ago by flack

(In [26401]) Reorder functions for better readability, refs #359

comment:17 Changed 7 years ago by flack

(In [26402]) remove some redundant code, refs #359

comment:18 Changed 7 years ago by flack

(In [26403]) remove some redundant checks, refs #359

comment:19 Changed 7 years ago by flack

(In [26404]) streamline parent/lastgoodnode code, refs #359

comment:20 Changed 7 years ago by flack

(In [26405]) cache loaded leaves per topic, refs #359

comment:21 Changed 7 years ago by flack

(In [26411]) re-implement NAP cache (still wip), refs #359

comment:22 Changed 7 years ago by flack

(In [26415]) a bit cleaner workaorund for the serialization/memcache bug, refs #359

comment:23 Changed 7 years ago by flack

(In [26422]) make leaf cache work correctly, refs #359

comment:24 Changed 7 years ago by flack

(In [26427]) avoid loading full topic from database where possible, refs #359

comment:25 Changed 7 years ago by bergie

  • Priority changed from major to blocker

comment:26 Changed 7 years ago by flack

(In [26440]) Added new dba proxy class which helps us avoid slow serialize/deserialize operations (and the id bug) in memcache that happen with regular dba objects. Also, change the code so that in the default configuration, m.h.nav & f.p.navigation don't need topic objects to work

refs #359

comment:27 Changed 7 years ago by jval

In the commit above you replaced

self::$_nodes[$parent_node][MIDCOM_NAV_OBJECT]->id

with

self::$_nodes[$parent_node][MIDCOM_NAV_ID]

but you should have replaced them with

(int) self::$_nodes[$parent_node][MIDCOM_NAV_ID]

Please correct that before you merge to official repo.

(MIDCOM_NAV_ID is not always, in case of a symlink target or target's subtree, the same as the storage object id. Casting it to int ensures it's the storage object id. Of course only cast to int if it's used as storage object id. Here it was used like that so we need to do it.)

comment:28 Changed 7 years ago by flack

(In [26445]) fix the last commit to work with the symlink feature, refs #359

comment:29 Changed 7 years ago by flack

(In [26450]) add noentry nodes to cache as well, in case list_noeds is called with the second parameter, refs #359

comment:30 Changed 7 years ago by flack

(In [26451]) cache node's subnodes, refs #359

comment:31 Changed 7 years ago by flack

(In [26452]) make sure NAP cache always gets fully populated & move privilege check to a better place, refs #359

comment:32 Changed 7 years ago by flack

(In [26454]) use correct variable name, refs #359

comment:33 Changed 7 years ago by flack

(In [26455]) skip ACL checks for admins, refs #359

comment:34 Changed 7 years ago by flack

(In [26456]) don't determine user_id each time we run get_leaves, we already know the answer, refs #359

comment:35 Changed 7 years ago by flack

(In [26457]) use dbaproxy for caching leaves as well. This is faster and allows us to remove the workaround for the serialize id-bug, refs #359

comment:36 Changed 7 years ago by flack

(In [26464]) merge NAP changes back to ragna branch, refs #359

comment:37 Changed 7 years ago by flack

(In [26468]) make sure pseudoleaves don't get lost, refs #359

comment:38 Changed 7 years ago by jval

(In [26474]) This is how call_user_func() is used with classes and methods. Fixes this for PHP 5.1. Refs #359

comment:39 Changed 7 years ago by flack

(In [26476]) Fix warning in sites with memcache disabled, refs #359

comment:40 Changed 7 years ago by flack

Note to self: There seems to be a bug where incomplete NAP entries get cached when the root topic is invisible to the current user

comment:41 Changed 7 years ago by flack

(In [26483]) lower priority of log message since it's not necessarily an error, refs #359

comment:42 Changed 7 years ago by flack

(In [26484]) fix corner cse where wrong data could end up in cache when list_nodes is called from invalidate(guid), refs #359

Diese und die folgenden Zeilen werden ignoriert --

M _basicnav.php

comment:43 Changed 7 years ago by jval

(In [26559]) Add lang to nap cache keys (to get correct titles on multilang environments), refs #359

comment:44 Changed 7 years ago by flack

  • Resolution set to fixed
  • Status changed from new to closed

r26484 seems to have fixed the incomplete cache entry issues, multilang and symlinks are working, and it's gotten a lot faster => closing as fixed

comment:45 Changed 7 years ago by jval

(In [26597]) Don't change existing behaviour (and break live production sites out there depending on the feature which has existed since r4604 - over 4 years). At least we are using the feature on public site (not just for editing purposes). The NAP cache works when this setting is enabled so there's no technical reason to change the existing behaviour. If you don't need this feature and want to improve performance even more, feel free to set this to false locally of course. Refs #359

comment:46 Changed 7 years ago by jval

  • Resolution fixed deleted
  • Status changed from closed to reopened

There are at least two bugs:

1) Even after r26597 I don't get CSS status classes in navigation (in sitemap it works but not in navigation for some reason).

2) Symlinks don't work. I only see leafs from symlinked topics. No child topics.

comment:47 Changed 7 years ago by jval

(In [26598]) Use correct navigation node IDs, refs #359

comment:48 Changed 7 years ago by jval

I know why bug number 2 happens: With symlinks the target is stored as MIDCOM_NAV_OBJECT and the old code used the target when topics were listed. The new code uses the symlink itself instead and naturally it fails then (we want to list topics from the target - not from the linking topic).

comment:49 Changed 7 years ago by jval

(In [26599]) If topic symlink feature is enabled list topics using the object ID so that topics get listed from the target, refs #359

comment:50 Changed 7 years ago by jval

Bug number 2 is now: Symlinks work only partially. Child topics are listed only from target but not below. So target's children aren't listed for some reason.

comment:51 Changed 7 years ago by jval

(In [26600]) Pass correct id and up to _loadNode(), refs #359

comment:52 Changed 7 years ago by jval

(In [26601]) It's a node ID - not a topic ID, refs #359

comment:53 Changed 7 years ago by jval

Bug number 2 seems to be fixed now but found another bug: When I take latest midcom_helper_nav navigation order doesn't work correctly when you view a node as symlink target.

comment:54 Changed 7 years ago by jval

(In [26602]) Pass correct guid when topic symlinks are used, refs #359

comment:55 Changed 7 years ago by jval

Okay, other issues are now fixed but this bug is still there:

Even after r26597 I don't get CSS status classes in navigation (in sitemap it works but not in navigation for some reason).

comment:56 Changed 7 years ago by jval

Ah, it's not true that it works in sitemap but not in navigation. It's a generic issue. I do get untranslated CSS class but not folder's classes. So for some reason r25760 stopped from working.

comment:57 Changed 7 years ago by jval

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [26603]) Don't check object's class. Even though page_class is only used for folders it can be generic in code. Fixes #359

comment:58 Changed 7 years ago by jval

I don't think the object_status_to_class property (which controls if CSS status classes are added to navigation items) in fi.protie.navigation actually makes any difference performance-wise (except for the very small execution time of the method which returns the CSS status classes). NAP always stores full object to the node data array anyway (it's already there and it has to be because that's how NAP was designed) and also NAP cache stores the full node data array where the full object is inside. Remember that with PHP 5.x passing already existing objects isn't slow because only reference/pointer is then passed in memory.

comment:59 Changed 7 years ago by jval

Btw. thanks for implementing this. :)

Sitemap's load times:

  • 8.09.9 ~8.5 seconds
  • svn HEAD: ~3.5 seconds

Nice performance improvement. Even though the page comes from the content cache in 0.xx seconds in 99% of the requests it's nice to know that when it doesn't it loads in a decent time. :)

comment:60 follow-up: Changed 7 years ago by jval

  • Resolution fixed deleted
  • Status changed from closed to reopened

It seems NAP cache doesn't invalidate when you edit a page.

comment:61 Changed 7 years ago by flack

Could you elaborate a bit more on the issue where cache doesn't invalidate when editing pages? I tested this many times during the implementation and never had a problem with it. How can I reproduce the issue?

comment:62 Changed 7 years ago by flack

(In [26683]) fix notice, refs #359

comment:63 Changed 7 years ago by jval

I'll try to produce this on test server later but it might be related to MultiLang. Perhaps you haven't tested the implementation with ML. I don't think I have tested it without ML at least so ML might be related to the issue.

comment:64 Changed 6 years ago by flack

About the cache invalidation problem: The NAP cache uses the delayed execution mechanism of backend.php, i.e. it calls $this->_cache->remove() instead of $this->_cache->remove(). So if $_MIDCOM->cache->shutdown(); is not properly executed before the end of the request, this might explain the problem.

comment:65 Changed 6 years ago by flack

Silly typo: NAP cache calls

    $this->_cache->_remove()

Try changing it to

    $this->_cache->remove()

If this fixes the problem, it means that $_MIDCOM->cache->shutdown() doesn't get called properly before the request ends

comment:66 Changed 6 years ago by jval

(In [26826]) Mark midcom_core_dbaproxy as final because we don't check in is_midcom_db_object() if classes extend midcom_core_dbaproxy. Refs #1942, refs #359

comment:67 in reply to: ↑ 60 Changed 6 years ago by jval

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to jval:

It seems NAP cache doesn't invalidate when you edit a page.

Ok. I retested this. It seems that wasn't related to the NAP cache at all. The same thing happens with and without the NAP cache.

If you change e.g. article title the navigation doesn't immediately show the new title. The second page load shows it. This happens probably because navigation is drawn before article is updated... (At least that would explain it.)

Anyway... That's not related to this new NAP cache feature so this ticket can be closed.

comment:68 Changed 6 years ago by jval

  • Resolution fixed deleted
  • Status changed from closed to reopened

NAP cache doesn't invalidate on destination and target folders when you move a leaf.

comment:69 Changed 6 years ago by jval

Sorry... I meant of course source and destination there.

Edits invalidate the cache properly it seems. But with moves there are two defects:

  1. Cache doesn't invalidate on destination when you move a node (folder/topic).
  2. Cache doesn't invalidate on source and destination when you move a leaf (e.g. article).

comment:70 Changed 6 years ago by jval

Also when you delete an article link, the folder's NAP cache view doesn't invalidate. So we have three defects:

  1. Cache doesn't invalidate on destination when you move a node (folder/topic).
  2. Cache doesn't invalidate on source and destination when you move a leaf (e.g. article).
  3. Cache doesn't invalidate when you delete an article link (e.g. net.nehmer.static).

comment:71 Changed 6 years ago by jval

I tested with latest svn HEAD so the current code (r26842) has those three issues.

comment:72 Changed 6 years ago by flack

(In [26843]) Fix some problems where cache would not be correctly invalidated when nodes/leaves are moved, refs #359

comment:73 Changed 6 years ago by jval

When all issues have been fixed we still need to make sure the code works with topic symlinks. It probably doesn't at the moment, so this is probably issue number four. :)

So don't close this until all four issues are ok and there are no known issues with the NAP cache.

comment:74 Changed 6 years ago by jval

(In [26844]) Ignore NAP cache invalidation requests with empty GUIDs, refs #359

comment:75 Changed 6 years ago by jval

Ok, normal node moves seem to work now and leaf moves almost work. Current issue list:

  1. Cache doesn't invalidate on source when you move a leaf (back and forth) the second time (e.g. article).
  2. Cache doesn't invalidate when you delete an article link (e.g. net.nehmer.static).
  3. Cache probably doesn't invalidate correctly when topic symlinks are used (judged from the code but not tested yet).

comment:76 Changed 6 years ago by jval

(In [26845]) Make the code easier to read/understand, refs #359

comment:77 Changed 6 years ago by jval

(In [26846]) Ignore all cache invalidation requests with empty GUIDs, refs #359

comment:78 Changed 6 years ago by jval

Normal nodes and leaves work after all. The first issue is also related to article links. Current issue list:

  1. Cache doesn't invalidate on source when you move an article (back and forth) the second time and there are article links pointing to the article (e.g. net.nehmer.static).
  2. Cache doesn't invalidate when you delete an article link.
  3. Cache most likely doesn't invalidate correctly when topic symlinks are used (judged from the code but not tested yet).

So this works with normal NAP objects but symlink handling needs work.

comment:79 Changed 6 years ago by jval

(In [26847]) Make NAP to return preferably the real object instead of a symlinked one when GUID is used, refs #359

comment:80 Changed 6 years ago by jval

Ok, now we should always have the real object when it's available, so symlinks shouldn't affect normal nodes and leaves. Current issue list:

  1. Cache doesn't invalidate when you delete an article link (e.g. net.nehmer.static).
  2. Cache most likely doesn't invalidate correctly when topic symlinks are used (judged from the code but not tested yet).
Note: See TracTickets for help on using tickets.