_xmlsitemap_set_memory_limit() is called only once in the current xmlsitemap module code, within _xmlsitemap_regenerate_before(). It is called with no arguments. The intention of _xmlsitemap_set_memory_limit() appears to be that you can call it with a specified memory limit (using the argument) and the memory limit will be bumped to the specified limit. Alternatively, you can call it with no argument, in which case the function will attempt to glean the appropriate memory limit using the xmlsitemap "chunk size" and, if alias prefetching is enabled, the expected size of the url aliases.
It looks like the current logic flipped an is_null check, and given the current implementation the memory limit will never be dynamically raised. We ran into this with a site that had alias pre-fetching on and also had a url_alias table running > 1M language-independent rows.
It's worth noting that we agree with the maintainer's suggestion of disabling alias prefetching on large sites, trading off more queries for a smaller memory footprint. The option just happened to be toggled on our instance, which led to the discovery of the bug.
- Fix the logic error in _xmlsitemap_set_memory_limit() so that dynamic memory limit calculation can occur.
- Enhance the memory limit heuristic so that it accounts for the current memory usage at the time of the calculation as the base for raising the limit.
Patch coming up in a follow-up comment.
None.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | memory_limit-1343018-9.patch | 2.06 KB | bdurbin |
| #6 | memory_limit-1343018-6.patch | 2.06 KB | bdurbin |
| #3 | memory_limit-1343018-3.patch | 1.36 KB | bdurbin |
| #1 | memory_limit-1343018-1.patch | 1.08 KB | bdurbin |
Comments
Comment #1
bdurbin commentedAttaching proposed patch.
Comment #2
Anonymous (not verified) commentedAre you sure you want to use memory_get_usage rather than finding the value of memory_limit with ini_get? Memory_get_usage returns the current value of memory allocated to the script and not the memory_limit set in php.ini. If you did mean to use memory_get_usage I would suggest using the $real_usage parameter and passing a TRUE value.
Needs work because you need to create the patch against the 7.x-2.x-dev version which then gets back ported to 6.x-2.x-dev.
Comment #3
bdurbin commentedThanks for the feedback. You're correct on using ini_get vs. memory_get_usage. I hadn't considered that the initial patch might have actually reduced the memory limit on sites with a small number of aliases and/or a small batch size. It looks like the existing _xmlsitemap_get_optimal_memory_limit() function, if it had been called, might have done something similar. Revised patch against 7.x-2.x-dev attached.
The new thinking is that while we can't account for how much memory other modules may consume during the request, as long as all other operations during the request would stay beneath the configured memory limit, we're raising the limit just enough to account for the memory xmlsitemap is about to consume.
Comment #5
Anonymous (not verified) commentedYou need to take into account a memory_limit value of -1 which means no limit.
Comment #6
bdurbin commentedIf I'm reading _xmlsitemap_set_memory_limit() correctly, you'll never get to _xmlsitemap_get_optimal_memory_limit() if memory_limit is -1. The re-rolled patch adds a check to _xmlsitemap_get_optimal_memory_limit() in case that function gets used in some other context in the future.
Comment #8
Anonymous (not verified) commentedShould read
Comment #9
bdurbin commentedBlerg. Thought I had corrected that before the last upload, sorry. Re-rolled.
Comment #11
Anonymous (not verified) commentedThe test bot failure appears to me to be more of a test function issue. It is related to taxonomy. Dave Reed can you take a look please?