Closed (duplicate)
Project:
Feeds
Version:
6.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
7 Apr 2010 at 09:56 UTC
Updated:
7 Apr 2010 at 15:32 UTC
When proceeding to very large file import batch, at save time (after the first 50 elements, with default configured limit value) the FeedsSource::save() method, at line 202, will save the source implementation separately.
I did not make any profiling, I will this afternoon, but I think the source is trying to save either the file buffer, either a lot of useless objects which fataly breaks the PHP memory limit.
A good thing would be to unset the file buffer from memory before saving, which would force the FeedsFileFetcher to read it again when loading the current state from database.
Another cause may also be the current batch items which represent a large memory amount (parsed RSS feed of, in my case, something like 1,500 items), when trying to save it in database, at some point, the full object tree is copied in memory and it breaks PHP doing to much memory allocations, somehow saving an offset rather than the current items tree could be less memory-killer (at a minor performance cost which is the need to re-parse the file at each batch run and manually remove each already parsed item until reaching the given offset within batch context).
Not sure about this statement, but It thinks these upper described statements may be the cause.
I might be totally wrong about the causes, I'll do some code introspection and review in order to find the real cause, but I think I'm not far from the answer.
What's annoying with this bug, is that the FeedsNodeProcessor works very well and the batch is limited to a low enough number of elements to process, so all are saved without any problems; It's quite irritating that it's at the context save time that all goes mad:)
Comments
Comment #1
pounardLooking a bit more to your code, I can ensure you that 1500 feed items stored in the serialized batch table database row is really too much. Items fetching should be incremental.
Comment #2
alex_b commentedThere's an issue for using batchAPI also on the parsing stage here: #744660: Expand batch support to fetchers and parsers.
Did you do any measurements on how much memory is being consumed by save() ?
Looking at the code, I'm seeing that save() is using an array to pass into drupal_write_record for saving.
There may be other points in the code where arguments are passed by value rather than by reference... Setting this issue to HEAD as it is not limited to alpha 13.
Comment #3
pounardThis is quite hard to profile, but if you want numbers, I'm parsing a 50mo file which contains 1500 items, using a 512mo of PHP memory limit is not enough at save time (I'm quite sure this isn't even 30% consumed when parsing the nodes).
Indeed, using as much as references could a lot improve the memory consumption!
The problem is the batch stores the raw data and the items, one of those should be dropped, even better both of them, the batch should be able to known its current offset and ask the parser to parse if its item array is empty.
I'm currently trying to do a patch that way, but the current feeds design makes the task quite difficult because the batch implementation does not know anything about the parser.
Comment #4
pounardI did this modification, to avoid (at least) one array copy.
This is not enough, here is the PHP trace when failing:
As you can see, it tries to allocate about 130mo of memory, which is, I must say, really a lot! If you consider that at least 50mo have been consumed by the file loading (buffer remains within the batch implementation $raw property), and consume about at least twice this size by storing the full item list array, this means with a 50mo file, the drupal_write_record tries to store about 50 + 100 = 150mo of data in only one row!
This is defective by design, parsers and batch implementation should really do incremental work, and batch table should never store any piece of these data, but only a small amount of data necessary to continue its work (things like source file and current offset).
Comment #5
alex_b commentedNo luck with
memory_get_usage()at the beginning and end of save().Have you tried it?
The raw should not be stored in most cases (actually only if you're using pubsub). What fetcher and parser are you using? I could think of a resetRaw() or similar method on a batch object that a parser can call after parsing is done.
This is clearly in the problem space of #744660: Expand batch support to fetchers and parsers, but let me respond to this as it came up here: I think the way to address the problem of large feeds is not to make batch objects aware of fetchers or parsers but to support batching on the parser stage:
This would be a modification to FeedsSource::import() and to parser plugins.
Comment #6
pounardI'm think about solutions, and the actual design is defective to do incremental parsing.
One solution could be to store the items in the FeedsSource implementation, not in the FeedsImportBatch implementation. This would leave the FeedsImportBatch as a single value object containing only useful information such as current offset and total number of items.
Another way would be to set the current parser as property of the batch, and if the parser supports incremental work (may be using an interface or a FeedsParser specialization, such as FeedsIncrementalParser) try to lazzy load the items when the processor calls the shiftItem() method.
Something like this:
In the FeedsImportBatch implementation should do the trick, then store a "reduced serialized" version in database.
EDIT: This kind implementation would be the less intrusive and would leave the actual non-incremental existing implementation working without any further modifications (exception in the FeedsSource::import() method).
Comment #7
pounardMarked as duplicate, see #744660: Expand batch support to fetchers and parsers