[IMP] snippets: move all work from parent to mp workers #137
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
cawo-odoo
force-pushed
the
master-imp_snippets_memerror_performance-cawo
branch
4 times, most recently
from
3cb6112
to
2c980fa
Compare
|
ok, I stopped the mattness. If anyone wants to review ... :-) |
cawo-odoo
force-pushed
the
master-imp_snippets_memerror_performance-cawo
branch
from
2c980fa
to
bd55b91
Compare
cawo-odoo
changed the title
cawo-odoo
force-pushed
the
master-imp_snippets_memerror_performance-cawo
branch
from
bd55b91
to
3a69c90
Compare
cawo-odoo
force-pushed
the
master-imp_snippets_memerror_performance-cawo
branch
from
3a69c90
to
9ba22c9
Compare
cawo-odoo
force-pushed
the
master-imp_snippets_memerror_performance-cawo
branch
from
9ba22c9
to
a697852
Compare
aj-fuentes
reviewed
Sep 23, 2024
cawo-odoo
force-pushed
the
master-imp_snippets_memerror_performance-cawo
branch
from
b6888a6
to
117114b
Compare
|
all comments resolved |
aj-fuentes
reviewed
Sep 23, 2024
aj-fuentes
reviewed
Sep 23, 2024
cawo-odoo
force-pushed
the
master-imp_snippets_memerror_performance-cawo
branch
from
117114b
to
b8eebed
Compare
|
𓃠 ? |
|
🐈 ? |
aj-fuentes
approved these changes
Nov 14, 2024
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
cawo-odoo
force-pushed
the
master-imp_snippets_memerror_performance-cawo
branch
from
b8eebed
to
0ee95a6
Compare
cawo-odoo
force-pushed
the
master-imp_snippets_memerror_performance-cawo
branch
from
0ee95a6
to
3a916ac
Compare
|
@nseinlet concerning our earlier coffee-machine meeting: wdyt about the fixup just pushed? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
To improve this, this commit
All in all, in my test case, this