Similar to #2247557: Additional fix to avoid unecessary load() calls, the performance of og_is_group() can be improved by replacing entity metadata wrappers with direct use of the field API.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Devin Carlson’s picture

Status: Active » Needs review
FileSize
1.06 KB

A patch to replace the use of entity metadata wrappers with direct use of the field API.

ezra-g’s picture

At least with the code flow on the website that motivated this patch, we're still doing problematic entity loads. Here's XHProf output with both the present patch and #2247557: Additional fix to avoid unecessary load() calls.

ezra-g’s picture

FileSize
416.74 KB
ezra-g’s picture

joachim’s picture

+++ b/og.module
@@ -1807,15 +1807,29 @@ function og_membership_access($op, $entity, $account = NULL, $entity_type = 'og_
+  list($id, $vid, $bundle) = entity_extract_ids($entity_type, $entity);

Minor point: we don't seem to need the $id or $vid, so save the assignment by doing list(,,$bundle).

amitaibu’s picture

Status: Needs review » Needs work
+++ b/og.module
@@ -1807,15 +1807,29 @@ function og_membership_access($op, $entity, $account = NULL, $entity_type = 'og_
+  if (is_array($items)) {

Thanks for the patch. I don't think we need this line as the OG_GROUP_FIELD is a single boolean field.

amitaibu’s picture

+++ b/og.module
@@ -1807,15 +1807,29 @@ function og_membership_access($op, $entity, $account = NULL, $entity_type = 'og_
-  return !empty($wrapper->{OG_GROUP_FIELD}) && $wrapper->{OG_GROUP_FIELD}->value();

Also, we do need to confirm the passed entity indeed has the OG_GROUP_FIELD

iamjon’s picture

Would running a query against the og_memebership table not provide the same results? IE check if the entity/enity_id is in the og_membership table as a gid it's a group. If not no?

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1009 bytes
775 bytes

Tried to address comments in #5 #6 and #7. Though #7 the existence will be there if the value is not false from field_get_items() I believe, no?

joelpittet’s picture

Issue tags: +Performance
joelpittet’s picture

Here is some xhprof results to go along with that:

And it's related to #2134365: OG's buildEntityFieldQuery/og_is_group() causes large # of entity loads leading to performance issues, but still if that one doesn't get in then this one is still a nice improvement.

azinck’s picture

Status: Needs review » Reviewed & tested by the community

Looks good; nice improvement. #9 RTBC

rv0’s picture

In my experience, the result of this patch on a site with ±1000 groups wasn't very impressive. Memory usage was identical or even slightly higher, page execution time was a bit lower, but not a lot. There's a lot of other places where the individual entities are loaded, like og_user_access. So I presume caching is the reason I don't see a big difference.

joelpittet’s picture

@rv0 try xhprof and see, I don't have nearly that many groups (maybe half that) and probably 3-5 group types and it saved nearly 1 sec on load time in the above report

amitaibu’s picture

Status: Reviewed & tested by the community » Fixed

Merged, thanks!

  • amitaibu committed 9d625a9 on 7.x-2.x authored by joelpittet
    Issue #2250177 by joelpittet, Devin Carlson, ezra-g: Improve performance...

Status: Fixed » Closed (fixed)

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

gbirch’s picture

For anyone who reaches this, there's a very subtle bug in this code that comes up in an edge case. And arguably it's more of a bug in views, but I found this first.

If you have a Group ID argument to your view, and you use the "Content" validator, the underlying plugin (views_plugin_argument_validate_node.inc) passes a node object to node_access(). But instead of loading up the full node, it just passes an object grabbed from the node table, without any fields. As a result, og_is_group() returns FALSE (in both the released and dev versions). This does not affect users who are the authors of their groups, but it means that group admins are denied access.

As a workaround: you can implement hook_node_access() in a custom module and do the following:

  // OG and views_plugin_argument_validate_node do not play nicely together.
  if (is_object($node) && '[YOUR GROUP TYPE HERE]' == $node->type && !empty($node->nid) && !isset($node->{OG_GROUP_FIELD})) {
    $full_node = node_load($node->nid);
    return og_node_access($full_node, $op, $account);
  }