Use multiprocessing when parsing packages. #170
Conversation
mikepurvis
force-pushed
the
parallel-pkg-parse
branch
from
33c4151 to
d2488fb
Compare
src/catkin_pkg/packages.py
Outdated
| for path in package_paths: | ||
| packages[path] = parse_package(os.path.join(basepath, path), warnings=warnings) | ||
| return packages | ||
| parser = _PackageParser(capture_warnings=isinstance(warnings, list)) |
There was a problem hiding this comment.
Should this check we changed to:
capture_warnings=warnings is not None
in order to allow "duck typing"?
There was a problem hiding this comment.
Yeah, probably— will change.
src/catkin_pkg/packages.py
Outdated
| parser = _PackageParser(capture_warnings=isinstance(warnings, list)) | ||
| results = multiprocessing.Pool(4).map(parser, package_paths) | ||
| if parser.capture_warnings: | ||
| map(warnings.extend, [package_warnings for _, _, package_warnings in results]) |
There was a problem hiding this comment.
Maybe use results[2] to avoid the underscores?
There was a problem hiding this comment.
Yeah, can do. I find the tuple unpacking a bit more readable than explicit indexing, but it could go either way.
There was a problem hiding this comment.
Okay, I've dropped the comprehensions in favour of zipping; I think that's maybe a bit clearer.
src/catkin_pkg/packages.py
Outdated
| packages[path] = parse_package(os.path.join(basepath, path), warnings=warnings) | ||
| return packages | ||
| parser = _PackageParser(capture_warnings=isinstance(warnings, list)) | ||
| results = multiprocessing.Pool(4).map(parser, package_paths) |
There was a problem hiding this comment.
Should we use something more "dynamic" like multiprocessing.cpu_count() as a default (and fallback to a hard coded number like 4)?
There was a problem hiding this comment.
Looks like you can just leave off the number and it will fall back on cpu_count as the default. Will change (I found empirically that 4 worked well for me).
mikepurvis
force-pushed
the
parallel-pkg-parse
branch
from
d2488fb to
a65b88f
Compare
mikepurvis
force-pushed
the
parallel-pkg-parse
branch
from
03d2852 to
9a0641a
Compare
|
This looks good. It might be a bit out of scope for the patch but it would be great if the API would allow the same improvement for this use (https://github.com/ros/rosdistro/blob/master/test/test_build_caches.py#L40-L41) which passes the xml rather than the paths. |
|
I tried to apply the same idea to the I tried calling the API |
|
There's certainly a cost to summoning the other processes. I just wasn't sure if the cost was enough to justify switching between implementations, but happy to go either way on it. |
|
I don't really have the time/energy for it at the moment, but it would be interesting to study whether the time in the kind of situation is predominantly being spent in syscalls, XML parsing, or Python somewhere. If it's either of the first two, then perhaps there are cases where the GIL isn't being properly released? If that was the case, and it could be fixed, then you'd definitely expect a threaded implementation to be the fastest, particularly since the setup cost then is way smaller. |
|
Maybe there's a cleaner implementation here where multiprocessing is only used to get them into memory in parallel, and then they're parsed serially in a |
|
I did some more benchmarking with a path containing ~1000 packages:
Therefore I have created an updated PR which uses With this implementation the performance with |
|
Interesting! I admittedly didn't dig in, but it definitely looked to me like the loading rather than parsing was the more expensive piece. Thanks for verifying that not to be the case. |
|
Maybe it also depends on what kind of system your are using? Does the new PR show the same improvements for your use case as this one? |
2 participants
As discussed in #169.
It's a nuisance having to put the wrapper/helper class up at module level, but multiprocessing can only pickle top level functions and objects, so I don't see an easy way around this. The class could potentially be avoided by defining separate helpers to always capture/not capture warnings, but that's a matter of taste.