Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#11 | XHProf__Hierarchical_Profiler_Report.png | 286.65 KB | joelpittet |
#9 | interdiff.txt | 775 bytes | joelpittet |
#9 | improve_performance_of-2250177-9.patch | 1009 bytes | joelpittet |
| |||
#3 | og_is_group-3.jpg | 416.74 KB | ezra-g |
#1 | improve-og-is-group-performance-2250177-1.patch | 1.06 KB | Devin Carlson |
Comments
Comment #1
Devin Carlson CreditAttribution: Devin Carlson commentedA patch to replace the use of entity metadata wrappers with direct use of the field API.
Comment #2
ezra-g CreditAttribution: ezra-g commentedAt 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.
Comment #3
ezra-g CreditAttribution: ezra-g commentedComment #4
ezra-g CreditAttribution: ezra-g commentedComment #5
joachim CreditAttribution: joachim commentedMinor point: we don't seem to need the $id or $vid, so save the assignment by doing list(,,$bundle).
Comment #6
amitaibuThanks for the patch. I don't think we need this line as the OG_GROUP_FIELD is a single boolean field.
Comment #7
amitaibuAlso, we do need to confirm the passed entity indeed has the OG_GROUP_FIELD
Comment #8
iamjon CreditAttribution: iamjon commentedWould 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?
Comment #9
joelpittetTried 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?Comment #10
joelpittetComment #11
joelpittetHere 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.
Comment #12
azinck CreditAttribution: azinck commentedLooks good; nice improvement. #9 RTBC
Comment #13
rv0 CreditAttribution: rv0 at Coworks.be commentedIn 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.
Comment #14
joelpittet@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
Comment #15
amitaibuMerged, thanks!
Comment #18
gbirch CreditAttribution: gbirch at Tech-Tamer, LLC commentedFor 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: