[IMP] snippets: move all work from parent to mp workers

[cawo-odoo]

In convert_html_columns(), we select 100MiB worth of DB tuples and pass them to a ProcessPoolExecutor together with a converter callable. So far, the converter returns all tuples, changed or unchanged together with the information if it has changed something. All this is returned through IPC to the parent process. In the parent process, the caller only acts on the changed tuples, though, the rest is ignored. In any scenario I've seen, only a small proportion of the input tuples is actually changed, meaning that a large proportion is returned through IPC unnecessarily.

What makes it worse is that processing of the converted results in the parent process is often slower than the conversion, leading to two effects:

1. The results of all workers sit in the parent process's memory, possibly leading to MemoryError (upg-2021031)
2. The parallel processing is being serialized on the feedback, defeating a large part of the intended performance gains

To improve this, this commit

• moves all work into the workers, meaning not just the conversion filter, but also the DB query as well as the DB updates.
  • by doing so reduces the amount of data passed by IPC to just the query texts
  • by doing so distributes the data held in memory to all worker processes
• reduces the chunk size by one order of magnitude, which means
  • a lot less memory used at a time
  • a lot better distribution of "to-be-changed" rows when these rows are clustered in the table

All in all, in my test case, this

• reduces maximum process size in memory to 300MiB for all processes compared to formerly >2GiB (and MemoryError) in the parent process
• reduces runtime from 17 minutes to less than 2 minutes

[robodoo]

[cawo-odoo]

ok, I stopped the mattness. If anyone wants to review ... :-)

[aj-fuentes]

This loos mostly good.

[aj-fuentes]

Some suggestion about the row processing method. Untested!

Regarding the test I'm OK with it, but you could put a comment to somehow clarify the extra steps in the test.

[cawo-odoo]

all comments resolved

[aj-fuentes]

cc: @KangOl

[cawo-odoo]

𓃠 ?

[cawo-odoo]

🐈 ?

[cawo-odoo]

@nseinlet concerning our earlier coffee-machine meeting: wdyt about the fixup just pushed?

[cawo-odoo]

rebased, conflicts resolved

[cawo-odoo]

@nseinlet concerning our earlier coffee-machine meeting: wdyt about the fixup just pushed?

Today I improved the comments about the backwards compatibility, rebased, squashed the fixups. Do I get a review? And/or a 🐈‍⬛ ?

[aj-fuentes]

We can make this simpler: if dbname we know is the new impl.

Suggested change

	def __call__(self, query):
	# backwards compatibility: caller passes rows in the "query" argument and expects us to return converted rows
	if not (self.dbname and self.update_query and isinstance(query, str)):
	return self._convert_row(query)
	def __call__(self, row_or_query):
	# backwards compatibility: caller passes rows in the "query" argument and expects us to return converted rows
	if not self.dbname:
	return self._convert_row(query)

I find this merged implementation unnecessary. The two interfaces cannot be used transparently by the same client code because we need the extra parameters in the new version.

In this case I think the good old OOP may be cleaner. For example, rename this class to ConvertorNG and code it for the query variant. Then make Convertor a subclass that keeps the old behavior.

class ConvertorNG:
    def __init__(self, converters, callback, dbname, update_query):
       ...
    def __call__(self, query):
       # filter rows if they do not have id, instead of by None
    def _convert_row(self, row):
       # do not return None if no id, use original impl


class Convertor(ConvertorNG):
    """Deprecated kept for compatibility use ConvertorNG"""
    def __init__(self, converters, callback):
        super().__init__(converters, callback, None, None)
    def __call__(self, row):
        return self._convert_row(self, row)

[cawo-odoo]

I took the suggestion to make it simpler. Sub-classing may be perfectly clean, but it is also a lot of additional loc. Do we want that?

[aj-fuentes]

I took the suggestion to make it simpler. Sub-classing may be perfectly clean, but it is also a lot of additional loc. Do we want that?

TL;DR: I'm OK with both variants.

It's not about LOCs, it's about the clarity in the implementation. If somebody out there is using the old interface we don't break them. We will always use the new one including this PR. IMO it isn't worth to have a double approach for us internally if we would never use the other alternative. I'm still OK with this variant of the code, but having to understand and read the "compatibility code" next time we need to debug the code is unnecessary if we can just move it out of the way.

IMO having a double use parameter query_or_row only makes sense if we will use both variants. Since we are not, I'd suggest a new class keeping the old one for compatibility. Note that we do that already in many other places, just that as function aliases, here we are modifying a callable class hence the subclass suggestion.

That being said. Do not change it just for me. Let's ping @KangOl for a final review :)