I tested the 6.x dev version and just when enabling the module i noticed a global speed descrease of the
whole site. Page requests was noticable slower.

Have other people noticed that too?
Any side effects to other modules?
I just building a site by testing modules under xampp on my localhost.
I had only OG and OG default roles installed.
Gallerix, Ubercart and the modules around it but nothing else dealing with
access.

CommentFileSizeAuthor
#65 og_content_type_admin_5.patch5.25 KBAnonymous (not verified)
#60 og_content_type_admin.patch5.25 KBNickSI
#55 og_content_type_admin.patch4.65 KBNickSI
#54 og_content_type_admin.patch4.69 KBNickSI
#16 ogcta.330135.patch579 bytesAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Assigned: Unassigned »

Thanks Michtoen,

This is probably to do with clearing the cache with every page request with og_content_type_admin_init() which was done so that og_content_type_admin_menu_alter() is called with every page load to ensure that access permissions are correct.

I'll refresh my memory and look into this again.

-Paul

criver’s picture

yep, I also experienced this with the 6-dev version, even just running on my local test environment.

I really like the functionality, but the page loads are taking a very long time.

Anonymous’s picture

I'm coming back to this later this week .

We probably need to have a complete rethink on how the content permissions are controlled
with this module.

If anyone has any ideas of course please post them here .

Thanks Paul

Anonymous’s picture

This is top priority for tomorrow!

Anonymous’s picture

Status: Active » Fixed

Fixed.

Anonymous’s picture

Status: Fixed » Closed (won't fix)

I can't seem to improve upon the method of implementing hook_init() to
clear the caches with each page request

/**
 * Implementation of hook_init().
 */
function og_content_type_admin_init() {
 drupal_flush_all_caches();
}

Interestingly i couldn't refine this approach by calling menu_cache_clear_all() this would not trigger the calling
of og_content_type_admin_menu_alter(&$callbacks) { .. } which would then adjust the access permission
based on the group context of the page.

I looked at the idea of of using the hook_init() to clear the caches conditionally by checking if if group context
had changed but there are corner cases introduced everywhere ie if you
introduce OG forum and click on the group menu link "Group Forum" then your taken to page forum/ which is recognised
as a site wide page which you could probably allow for but then you get a similar problem with OG calendar and the
problem grows and grows

So i am open to suggestions here how to resolve this problem permanently.

I have reverted back to how it was before which seems to work fine on my local system

Best, Paul

Flying Drupalist’s picture

Hi, I'm a little confused whether or not this is actually fixed. I just tried the beta and was experiencing this.

Anonymous’s picture

The perfomance issue still remains.

It looks as though we need to clear the cache with each page request in order to cause the drupal machinery to call og_content_type_admin_menu_alter(&$callbacks) { .. }
and adjust the permissions according to the group context of the current page.

I don't know how to fix this problem at the present time so will need to continue to investigate .

Paul

Anonymous’s picture

Status: Closed (won't fix) » Active
Anonymous’s picture

Assigned: » rconstantine

@Ryan

Do you have any thoughts how to remove this performance issue ?

Best, Paul

rconstantine’s picture

I'll give this a look. I've been reading a lot about the D6 APIs. My first thought it that we may not need to clear every cache, but could target one. Not sure. Also, there may be other menu hooks available, but I don't recall off of the top of my head.

Anonymous’s picture

@Ryan

Taking a look at this problem again. Did you have any time to look into this ?

Anonymous’s picture

It looks as though there are two things we might be able to do .

1. Rebuild permissions only when the group context changes
2. Clear only relevant caches instead of clearing all caches (if that is possible)

Investigating ..

rconstantine’s picture

There is a menu_cache_clear function.

See here http://api.drupal.org/api/function/menu_cache_clear/6

I think all we need is that, right? Start by changing that. Was there another cache that's affected?

As for rebuilding perms... your idea has merit. Where in the code are we doing it currently and how often. (I don't have the code in front of me right now.)

brisath’s picture

Subscribing

Anonymous’s picture

FileSize
579 bytes

I'll investigate the caches further later today and take some notes.

I have uploaded a patch which implements the strategy of rebuilding access permissions only when the group context changes and clearing only the menu cache.

Currently we clear all drupal caches with hook_init() with each page load because this was the only approach which would trigger the calling of hook_menu_alter which would then rebulid the access permissions based on the group context of a particular page.

function og_content_type_admin_init() {
 drupal_flush_all_caches(); //TODO: Need to remove the need for this
}
function og_content_type_admin_menu_alter(&$callbacks) {   // $types used twice!!!!!
  global $user;
  $post_type = arg(2); 
  // First: Deny access to all content types and handle all create content pages.
  $types = node_get_types();
  foreach ($types as $type) {
  	$type = $type->type; 
  	$node_type = isset($type) ? str_replace('_', '-', $type) : NULL; 
    $type_path = "node/add/$node_type"; 
    $callbacks[$type_path]['access callback'] = FALSE;
    $callbacks[$type_path]['page callback'] = 'og_content_type_admin_node_add';
  } 
  /*TODO: Would something like the below work above ? 
   *$callbacks[]['access callback'] = FALSE;  
   *$callbacks[]['page callback'] = 'og_content_type_admin_node_add';
   */
  $callbacks['node/add']['page callback'] = 'og_content_type_admin_node_add'; 
  // Second: Allow access to add a particlar type of group content 
  if (is_array($_GET[gids])) {  
    $gid = $_GET[gids][0]; 
    $sql = "SELECT octa.types_active, octa.types_allowed FROM {og_content_type_admin} octa WHERE octa.gid = %d";
      //if we're keeping track of this group, get it's active types, otherwise, get the defaults
      $types = db_fetch_object(db_query($sql, $node->nid));  // TODO: Should this be $gid 
      if (!count($types->types_active)) {  
        $types = db_fetch_object(db_query($sql, 0));
      } 
      $activated_types = unserialize($types->types_active); 
      foreach ($activated_types as $type => $value) {
         $node_type = isset($type) ? str_replace('_', '-', $type) : NULL; 
         if ($node_type == $post_type) {
           $type_path = "node/add/$node_type"; 
           $callbacks[$type_path]['access callback'] = TRUE;
         }	
      }	
  }  
  // Third: Allow access to site wide content ..  
  $sql = "SELECT octa.types_active FROM {og_content_type_admin} octa WHERE octa.gid = -1";  
  $types = db_fetch_object(db_query($sql));
  $activated_types = unserialize($types->types_active);     
  foreach ($activated_types as $type => $value) { 
  	$node_type = isset($type) ? str_replace('_', '-', $type) : NULL; 
    $type_path = "node/add/$node_type";
    if ($activated_types[$type] && node_access('create', $type)) {  
       $callbacks[$type_path]['access callback'] = TRUE;	
    }
  } 
}
Anonymous’s picture

The above patch does not work. Investigating ..

Anonymous’s picture

The modifications made to og_content_type_admin_init() (please see below) look to be an improvement, so i have committed the changes to the development branch so that the module can be tested quickly by everyone

function og_content_type_admin_init() {
  menu_rebuild();
  node_types_rebuild();	
}

It should be possible to add some further logic so that these core menu_rebuild() and node_types_rebuild() functions
are only called when the group context changes or when we are on a /node/add .. page that is creating group
affiliated content but i haven't been able to get that working.

Paul

rconstantine’s picture

Good job Paul. I just had some time, so I came to look to see how things are going. But it seems you may have got it. I'm sure it's much better to not clear any cache. IIRC, menus are stored per user, so rebuilding just their own menu seems the way to go.

I haven't had time yet to really get back into the module like I thought I would, so sorry I wasn't more help.

Anonymous’s picture

Thanks Ryan.

As soon as i get confirmation from two site developers that the changes have had significant positive effect on the performance of their sites ill then put out a further beta release

Anonymous’s picture

Assigned: rconstantine »
brisath’s picture

I upgraded to the recent dev version and subjectively there seems to be much improved performance now. Thanks for your work on this. The module is perfect for my needs. I'm having some problems with add-content links generating sporadic "access denied" errors when they shouldn't be. Should I suspect this module as the culprit?

Anonymous’s picture

Status: Active » Fixed

Please reopen if you experiencing any further performance issues in the development branch / BETA 3.

rconstantine’s picture

One more thought...

Did you look at this function? http://api.drupal.org/api/function/menu_cache_clear/6

You could target just the menu in question, though perhaps we don't know what menu that is at runtime since the admin can change things?

Alternatively, if menus are cached per user, then you could probably use cache_clear_all to target just that user's menu. There are three args that can be passed in which would allow for removing just one row from the table. It may be, however, that your method already does that for the current user. I'd probably open the DB table, run your method to see just what the rebuild does, and if it wipes out the menus for all users, I'd look into this function instead.

michtoen’s picture

Hello

I tested the last release with devel module and checking the performance.
Sadly, the results was still pretty worse.

With enabled module, and a fresh installed OG module, the main page had a generating
time of >1 second! When i disabled the type administration the access was around 50-90ms.

Anyone with different results?

michtoen’s picture

Status: Fixed » Postponed (maintainer needs more info)
Anonymous’s picture

It looks as though we can improve matters by changing

menu_rebuild();

for

menu_router_build(TRUE);

It looks as though the OG CTA module now seems to give a performance
hit of around 25 / 50%

Again , ill commit this to the development branch as it looks to be another
step in the right direction

Please let me know how you get on :-)

Best, Paul

Anonymous’s picture

Status: Postponed (maintainer needs more info) » Needs review
Anonymous’s picture

Status: Needs review » Closed (fixed)

Please reopen if your experiencing any further performance issues in the development branch / BETA 3.

pvasener’s picture

Version: 6.x-1.x-dev » 6.x-1.0
Status: Closed (fixed) » Active

I'm in the process of migrating my site to D5 to D6 and I'm using OG CTA 6.x-1.0. Unfortunately, I can confirm you that the performance issue is still there. Here are details without OG CTA:
Page execution time was 559.67 ms. Executed 381 queries in 141.87 milliseconds.
And now with OG CTA 6.x-1.0:
Page execution time was 2629.57 ms. Executed 1613 queries in 764.51 milliseconds.
Being curious, I reproduced the same tests on my D5 site and I didn't notice any sensitive overload. At least, nothing compared with the D6 version.
I can also confirm that the overhead comes 100% from the hook_init. From what I understand, the two function in here are used to generate the menus and node types but does it need to be called at every page load? I think it would be smarter to called these functions only when modules configuration is modified. Maybe in the _og_content_type_admin_rebuild_table function ? I will keep on investigating, especially how the D5 version handled this with no overhead.

SocialNicheGuru’s picture

Here is an issue around performance for OGCTA

http://drupal.org/node/311626#comment-1395178

highvoltage’s picture

Has there been any progress on this issue or does it have everybody stumped?

SocialNicheGuru’s picture

I'd love to use this module but the performance hit is extreme.

highvoltage’s picture

So I read through that link, and although all the technical drupal talk is out of my understanding, it seems like this is related to a problem with drupal itself and is not so much a problem with the module? Is it both?

Anonymous’s picture

Assigned: » Unassigned
EvanDonovan’s picture

Title: performance issue » menu_rebuild call in hook_init is a serious performance hit
Category: support » bug
Priority: Normal » Critical

menu_rebuild() was never intended to be called on every single page load, since it is one of the most expensive operations in Drupal currently. (See core issues #311626: admin/build/modules very slow on some configurations (which references this module specifically), #193366: execute various rebuilds when modules are submitted, #251792: Implement a locking framework for long operations, #306316: Regression: node_types_rebuild should rebuild menu, #302638: No-op and slow queries during menu rebuild, and #512962: Optimize menu_router_build() / _menu_router_save(), among others.)

I think it would be better to have your menu_alter() function change the access callbacks for the 'node/add/content-type' pages so that the access callback function checked to see whether the current user had access within the current organic group context. You would have to define users' access per organic group either by adding new permissions to the permission table, which would be checked via user_access(), or else by creating your own table, admin form & function to check access by organic group.

In any case, simply changing the access_callbacks to FALSE in hook_menu_alter() is a technique that will bog down all but the smallest of sites.

gmclelland’s picture

subscribing

jmpalomar’s picture

Subscribing.

ajayg’s picture

Any updates on this?

Please pardon my ignorance here. But just wondering what is the purpose of offending call (menu_rebuild) here? And if this is expensive call why the same (or equivalent call to similar API) worked fine in 5.x?

Is there a different way to achive the same with other means possible?

EvanDonovan’s picture

menu_rebuild() is being called to change the access permissions on certain core pages in Drupal (the node add & edit forms). There was no direct equivalent in Drupal 5.x, as the Menu API was substantially different.

I made a few suggestions in #36 on how the menu_rebuild() on every page load could be avoided. I think that the override of the access callback for the node add/edit pages would be the best way to move forward.

I would also suggest that the maintainers of the module look at the Spaces & Spaces OG modules, since they seem to achieve some similar functionality, but without the severe performance penalty.

ajayg’s picture

Status: Needs review » Active

So correct me if I am wrong (as of current version of this module) menu_rebuild is being called only on node add and edit forms which means only when OG member click to create new OG content or edit OG content? Corrrect? And NOT on every page view of the already created content? It does not solve the problem but atleast reduces its impact.

EvanDonovan’s picture

No, it is being called on every page request, as far as I know. The hook_init() doesn't have any conditionals in it to check for paths. Adding a conditional might help to mitigate the problem, but it would still, in my opinion, be a serious issue.

ajayg’s picture

Status: Active » Needs review

Based on above discussion with EvanDonovan here is a workaround I found working. Please review.
Please change following code in function "og_content_type_admin_init"

From

function og_content_type_admin_init() {
    menu_router_build(TRUE);
    node_types_rebuild();
}

To

function og_content_type_admin_init() {
  $cpath=$_GET['q'];
  $patn= '/node\/add\//';
  if (preg_match($patn,$cpath)) {
    menu_router_build(TRUE);
    node_types_rebuild();	
  }
}

This will call the code having performance issue only when you are creating a node. I also noticed checking for "add" is sufficient and edit works fine without calling menu_router_build.

Please review. I am hoping atleast this is immediate relief to few folks.

EvanDonovan’s picture

Title: menu_rebuild call in hook_init is a serious performance hit » menu_router_build call in hook_init is a serious performance hit
Status: Active » Needs review

Thanks, ajayg. I'm not actually using the module myself, so I can't review, but it looks like it should make somewhat of an improvement, although to change access callbacks would be much better.

Just realized that the module is called menu_router_build, not menu_rebuild itself. Since menu_router_build is called by menu_rebuild, it was an easy mistake to make.

From looking at the code for menu_router_build in menu.inc, it might actually make more sense to use menu_router_build() without the TRUE parameter, since that would mean that each module would not have to be invoked on every page load to get its menu items and add them to the $menu array. This would need testing of course.

JamesK’s picture

Status: Needs review » Needs work

Once this patch #251792: Implement a locking framework for long operations gets applied to core, you will need to use menu_rebuild() rather than menu_router_build() in og_content_type_admin_init() so that the locking gets applied.

ajayg’s picture

@Jamesk,
even then I would still question use of menu_rebuild. Looks menu_rebuild or menu_router_build meant for more like during module install or configuration (like once in a while). The way it is used in the module currently, it is getting called for every page view and even if you use my patch above on every node creation. IMHO that may not be the correct(or scalable) way to use those functions in this module's context. The issue you mentioned will fix the locking but it won't change the fact menu_rebuild is inherantly expensive operation not meant for every page view.

JamesK’s picture

@ajayg
It would be great if this module could remove its dependence on menu_rebuild() completely, but for now I think your patch + #251792: Implement a locking framework for long operations is as good as we'll get without getting buried in some sort of huge re-code of the module.

EvanDonovan’s picture

"The issue you mentioned will fix the locking but it won't change the fact that menu_rebuild is an inherently expensive operation not meant for every page view."

Exactly! See what I suggestion in #36, 2nd paragraph, for an alternative implementation. Or just use the OG Context module, which I think could accomplish the same thing.

ajayg’s picture

I have suggested a more simple approach here #533344: Use hook_og_links_alter() to remove content types from og group details block based on og_links_alter. At a very simple level I suggest a function which filters out the links based on different groups. Currently I implemented this filter in a custom module, but it can be modified to get this information from CTAOG's persisted data in the database.

markus_petrux’s picture

Have you tried with hook_translated_menu_link_alter() ?

Or maybe you could provide a custom access callback to node/add pages? This access callback would also invoke node_access('create') internally, in addition to OG related stuff.

Anonymous’s picture

"I think it would be better to have your menu_alter() function change the access callbacks for the 'node/add/content-type' pages so that the access callback function checked to see whether the current user had access within the current organic group context. You would have to define users' access per organic group either by adding new permissions to the permission table, which would be checked via user_access(), or else by creating your own table, admin form & function to check access by organic group."

This is very much my position. I always thought the approach of building & then throwing away the access permissions based on the node_access table and then rebuilding the permissions on the fly outside of the node_access tables was wrong and that the access permissions
should always be written to the node_access table.

However I'm not sure now why essentially the same approach that was used in D5 is such a performance hit in D6.

Some good new we have a bounty from one of my clients for $800 so if your interested in helping me to fix this issue and would like
to earn some money at the same time, please let me know.

Best, Paul

EvanDonovan’s picture

Thanks for letting me know! I'll try to have something preliminary, proof-of-concept-ish for you by the end of Saturday.

ajayg’s picture

>However I'm not sure now why essentially the same approach that was used in D5 is such a performance hit in D6.

I am newbie and I could be wrong here. But primary reason of new menu system is to make it efficient so instead of building on the fly (like on D5) it is now build once and cache for future use so it is efficient but breaks the logic we had earlier in D5.

NickSI’s picture

FileSize
4.69 KB

I've encoutered same performance problem and has followed idea from #50 (substituting access callback).
Patch attached

NickSI’s picture

Status: Needs work » Needs review
FileSize
4.65 KB

Sorry, found an error in the patch. Here is the new version

Anonymous’s picture

Thanks for the work done here.

Taking a look now ...

EvanDonovan’s picture

Well, looks like that should probably work. Guess I wasn't quick enough this time :)

markus_petrux’s picture

+  node_types_rebuild(); // moved from init function. Do we really need it?

Ditto. Do we really need it?

Anonymous’s picture

This doesn't work for me i get permanent incorrect 'access denied' pages. However i also seem to have a problem before applying this patch where a refresh is required to remove an incorrect 'access denied' on node creation pages.

Having another look at the patch as i couldn't quite work out why it would work ...

Best, Paul

NickSI’s picture

FileSize
5.25 KB

Hmmm, I gave it another look and found that I've misplaced allowed and activated types in one more place. New version attached.

Although I fail to see why this patch would not work. It uses very strightforward approach:

1. menu_alter hook replaces access callback (instead of making it FALSE). The access call back arguments will have all arguments from original callback as well as original callback name. Of course, you have to manually rebuild your menu if you are patching already installed module.
2. Each time user access menu, call back will be fired and check if gid is present in the url (the same way as an original code). If not, it will check if type is allowed and also fire original callback.

Anonymous’s picture

I did a quick test and it doesn't work. Taking a look at the patch ...

Anonymous’s picture

This looks to be working with reintroduction of the second line below.


$type = array_shift($args);
$type = isset($type) ? str_replace('-', '_',  $type) : NULL; 
$original_callback = array_shift($args);

More testing ..

markus_petrux’s picture

Regarding the following...

+  node_types_rebuild(); // moved from init function. Do we really need it?

I believe it is not really needed, at least for the login that remains here related to access callback once the patch is applied.

Anonymous’s picture

Also it looks as though we can do the following ..

 $active_types = unserialize($types->types_active);
    if ($active_types[$type]) {
      return TRUE;
    }	

.. seems to work fine :-)

Thanks for a great fix, this really puts this module back on track now.

Anonymous’s picture

Hi Folks,

Let me know how you get on with this patch. It would be great to get this work committed to the development branch today.

Thanks for your contributions everyone. I really couldn't have got past this problem without your help :-)

Best, Paul.

Anonymous’s picture

Pushing this work to the development branch.

Anonymous’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.