OG's buildEntityFieldQuery/og_is_group() causes large # of entity loads leading to performance issues

We could stop babysitting, and remove

  if (!og_is_group($group_type, $gid)) {
    // Not a group.
    return NULL;
  }

Could you try and benchmark without those lines?

OOPS, check the next comment.

I'm not too familiar with OG, so forgive me if this has issues.

I tested this locally and it seems to work, but I haven't tested performance.

I saw the entity reference loading in og_node_access() and tried to add in the check for og_context(). I'm not sure if this is correct. Is there another way that that loading / checking is occurring?

Ok, I think this patch should be good. I messed up the $type piece before.

Any thoughts?

I bench marked without the lines as per Amitai's request. It helped a smidge.
My page loaded at 39.6 seconds originally (local server, using a slow db and a user with 400+ groups) After the changes the page loaded at 34.1 seconds
Changing status

While I agree that this patch improves performance quite a lot, the side effect is that the "Group" tab will now appear on all content, even content thath is NOT a group... Any way to perform the check without loading the entire entity ?

Any update on this issue? It seems it stalled a couple of years ago but seemed close to getting committed.

I've hit an issue with a large Drupal site I manage that has 300+ groups and 6000+ users. Just done a big project to upgrade from OG1 to OG2 and got it working, but for a user that is a member of a lot of groups (around 80) then the site runs very slow, and they are unable to open the form to post a new node to a group as leads to a "PHP Fatal error: Allowed memory size" error.

Using the xdebug profiler can see that on an example page load:

og_user_access is called 84,731 times
og_is_group is called 84,798 times

When applying the patch in #4 (ie removing the call to og_is_group() from the og_user_access() function) then the performance definitely improves to the point where pages load ok and the add node to a group form loads without the site crashing. It is still a lot slower than if I give this user the site admin role (which then leads to almost instant page loads), but a lot better.

So would be great to know if the patch in #4 is safe to use and also if it could be committed.

@Vali Hutchison
Those numbers are quite impressive. Are you using any other OG contrib modules?

@rv0 - Thanks for the suggestion - I'd tried switching a lot of different modules but somehow missed this one - og_moderation - and this is the culprit. Disabling this module and everything runs nice and fast. So will post on the OG Moderation issue queue.

Would still be interesting to know if the above patch is going to be considered for OG as does seem to speed up the site and as suggested seems to be a potentially extraneous check.

RTBC++

+1 on #5 RTBC

I'm not seeing the problem in #7, unless I'm misunderstanding...

Actually, I do see the problem in #7. I was initially testing on a site with significant customization and we'd customized the access callback for the group tab so I wasn't seeing the problem until I tested in a different circumstance.

This is a pretty significant change in the behavior of the API. I think a safer update would be to add an optional argument to og_user_access called $bypass_is_group_check. Set it to FALSE by default but this way we can bypass the check if we know it's not necessary yet ensure we maintain backward-compatibility.

Ok, I've tried to do what I suggested in #13. I'm bypassing the og_is_group() call whenever I am confident that what we have has already been confirmed to be a group. This is essentially a completely new patch so no interdiff with #5.

@azinck, I think it may be easier and ideal to remove it and fix that one problem instead of trying to add a BC layer. Just my thoughts on the matter. Thanks for putting thought behind it and trying your idea out though

@joelpittet: but there are other side-effects of the removing the is_group check.

For instance: I have a Panels page that's only relevant to OG group nodes. It receives a node ID as one of its arguments and has a set of access checks that are combined using "or" logic. These access checks all use og_user_access() so they enjoy the benefit that access is never granted if the provided node ID isn't a group. Unfortunately, with the patch in #5, the logic here is now different and my page appears as a task on all nodes, not just on group nodes. I'll have to write a custom ctools access plugin in order to achieve the logic I need.

Anyhow, I don't mean to write a sob-story, but just to point out that #5 does, in fact, change the behavior of the API. This is a potentially breaking change for the who-knows-how-many people are relying on the current behavior of this function.

Might be wrong, but I believe you're all a focussing a bit too much on the og_is_group call, while the real venom lies in the tail if the buildEntityFieldQuery function imho
og_user_access is called an insane amount of times (for every group in some cases) and guess what that function does.. entity_load_single. if you have 1000 groups, that means 1000 entity_loads on node/x/edit of group content. Please correct me if I'm wrong.

@rv0 Do you mean a single a user is a member of 1000 groups? If that is the case, then it's not a typical one.

@amitaibu
yes, a single user can be a member of 1000 groups in worst case. (it's a school website, courses and activities are organic groups, old courses remain online so the number grows every year, staff members are usually in a lot of groups or need access to all of them). I currently "fixed" my performance issue by writing my own selection handler which limits the available group to the context (either from url or og_context, combined with entityreference_prepopulate)

Although I agree that 1000 groups is not a typical case, I guess for most people there's still a reasonable amount of groups getting loaded every time, so the og_is_group call is only part of the problem imho.

Sometimes it's hard to visualize whats actually going on in the background, Placing a dpm($nodes) in hook_node_load opened my eyes ;)

@amitaibu
RE: the 1000 groups thing, you said yourself in the following issue that OG can scale to more than +10000 groups without an issue:
#1477868: OG Scalability

> more than +10000 groups without an issue:

And I stand behind it, but not behind 10K memberships without any form of optimization :)

I also think that a user shouldn't be a member of so many groups. What we are doing in similar cases is alter the access of such users to let them interact with the group -- but without being a group of it. As often time, they are not really group members, but rather passing admins. Of course you use case might be different.

@amitaibu I'm also in that same boat that @rv0, as I'm working for a University. I think you are right at least for me, there is passing admins that need to be a member of groups.

What we are doing in similar cases is alter the access of such users to let them interact

OT: How are you doing this, I'd like to do the same!

@joelpettit: I have a patch here that does this https://www.drupal.org/node/1946424

I honestly forget the nature of the widget issues I ended up running into; they were not a problem for our use-case, but whatever the case you can at least use the patch as an inspiration.

Cool thanks for sharing @azinck

> OT: How are you doing this, I'd like to do the same!

og_user_access_alter()

Another solution we had for Harvard's open-scholar there was a specific request not to make those admin/ support team permanent members. So they are able to self subscribe, and then they are kicked out of the group after 3 days. This can be done by having an OgMembership type dedicated for "special" users

Oh, and all the folks here are invited to help with OG8 :)

Update og-og_is_group_remove-2134365-14.patch to be compatible with OG 7.x-2.10 version