Our site, for better or worse, has 3000+ beans--many of which use field collections. Modules like Panelizer and Block Reference invoke hook_block_info() on every uncached page. We are experiencing major performance issues because even with memcache and entity_cache installed, bean_block_info() spends a LOT of time just to provide the bean delta, title and CACHE_ constant.
Add persistent caching to bean_block_info() to avoid loading all beans at run-time when using panels.
Note that Drupal 8 block content entities suffer from a similar issue:
https://github.com/vasi/block_access_records
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | bean_unnecessarily-2734693-17.patch | 979 bytes | joseph.olstad |
| #3 | bean_block_info_xhgui_patch.png | 357.13 KB | sylus |
| bean-optimize-bean_block_info.patch | 528 bytes | krlucas |
Comments
Comment #2
sylus commentedsee next post.
Comment #3
sylus commentedI saw this too when profiling and gave the patch a try. I attached what got reported in xhgui + xhprof between comparison runs with patch not applied, then applied.
Comment #4
sylus commentedComment #5
sylus commentedWould love to get a opinion of the bean maintainer if this suffices?
Comment #6
damienmckennaComment #8
sylus commentedI fail to see what is causing the tests to break but they have been passing as of a week ago.
Comment #9
krlucas commented@sylus
I also don't know exactly why the patch is failing but per my original description the patch completely sidesteps the bean-as-ctools-plugin API enhancements that were semi-recently added to the Bean module API. Getting this patch to a committable state probably requires enhancing that API.
Your profiling results are nonetheless extremely valuable!
Comment #10
joseph.olstadCould the difference be that the testbot is now using Drupal core 7.44 ?
last successful pass of this patch was June 7th prior to the release of 7.44.
most recent pass, June 7, 2016 - 11:27
Comment #11
fabianx commentedThe best idea likely is to cache the hook_block_info() information as the array is sparsely populated anyway and only needs to be expired if a bean is saved (inserted / updated). It could automatically be expired when the _block_hash function is called - similar to how block_cache_alter() achieves the block altering, the cache could be expired then.
Comment #12
joseph.olstadJust triggering test bot
HEAD is passing
Comment #14
fabianx commentedAnd here we go, attached patch works like a charm for me even with many many beans and saves 250 ms per page load for me (out of 1.5 seconds).
The real modules to fix here is ctools content types though and context and ...
It is a real problem how many modules re-implement listing and rendering of blocks (and not using core blocks tables for it - even if block module is enabled).
Comment #16
fabianx commentedComment #17
joseph.olstadhip shot
Comment #18
joseph.olstadComment #19
joseph.olstadI wonder if the @krlucas patch and patch #17 could be combined for an even greater performance gain.
However I'd be inclined to RTBC #17 and get #17 committed as-is and open a new followup issue for investigating further possible improvements in performance using a rerolled @krlucas patch
Comment #20
joseph.olstadComment #21
joseph.olstadsee comment #19 , to me this seems like something that should urgently be done. Pet peeve on unnecessary lag.
Comment #22
krlucas commented@joseph.olstad
Yeah I was about to mark this as needs work because while @fabianx's caching helps n+1 calls to hook_block_info it doesn't really solve the "unnecessarily" part of the issue. AFAIK there are very few places in core that call hook_block_info EXCEPT after a block (or bean) has been saved. That first call too bean_block_info after saving a bean is still going to be really, really slow (on a site with a lot of beans) for no good reason. In my use case, the real burden of this issue fell on content authors, making their job unbearable.
That said, if we want to add this caching I won't object as long as a follow-up is created to address the real issue.
Comment #23
joseph.olstadwell if its not going to cause another 7 months of delay perhaps we should set this back to needs work then and combine your @krlucas patch with the patch #17
Comment #24
joseph.olstadComment #25
fabianx commentedDefinitely not. Lets discuss in a follow-up, because it is a quite complex issue:
The problem is that the bean adminTitle can be arbitrarily defined at run-time and also the cacheLevel property can be defined to something else than CACHE_PER_ROLE, e.g. the original patch would create problems on real sites.
Now we could "cache" those per bean e.g. by adding new properties to the database table and hope that no-one sets the adminTitle / cacheLevel dynamically.
Then the approach could in fact work, but its way more complicated than to add caching for now.
Comment #26
fabianx commentedAlso e.g. as soon as you use beans with panels instead of core block module, hook_block_info() is called uncached on every page load.
Comment #27
fabianx commentedFollow-up created here:
#2836128: Bean unnecessarily fully loads all bean entities on hook_block_info
Comment #28
joseph.olstad@Fabianx , nice work thanks. Lets get this in!
Comment #30
damienmckennaCommitted. Thanks.
Comment #31
joseph.olstadFor the record, credit should be to @Fabianx as author, I merely re-rolled it due to a broken path in the patch. Thanks @Fabianx and @DamienMckenna.
Comment #33
damienmckennaThanks for the clarification, joseph.olstad, I've recommitted the patch and gave Fabianx the credit.