kmod: eliminate duplication in calls to modinfo - #4392
Conversation
|
Thanks for the contribution! I will only have time to look at this later this week, but I started the CI run to see whether it works. |
behrmann
left a comment
There was a problem hiding this comment.
Thank you! Sorry for the lag on my end.
e9a067c to
2528c88
Compare
|
I've sat down an ran a tiny bit of benchmarking on this and running this against my host system, with empty inputs as required_kernel_modules(kver, modules_include=[], modules_exclude=[], firmware_include=[], firmware_exclude=[], resolve_func=resolve_module_dependencies)after making the function to resolve module dependencies pluggable to easily compare this, I get roughly the same run time for both functions, with the version from this PR actually a tiny bit, but consistently, slower. At first I thought it was the chunking and I changed that to which is roughly That helps a bit, but doesn't get one all the way, but the real trick is in lifting the todo update Since Could you check that this makes sense and works in your usecase? |
aa43b78 to
e2594b6
Compare
|
Hi and thanks for looking at my PR. How much benefit this does in practice depends both on the specifics of your With your change I see smaller 0-10% faster here, and your version is also cleaner so I'm happy to include your suggestion. I also added a Co-Authored-By. |
6a39f2b to
cdc37a6
Compare
martinpitt
left a comment
There was a problem hiding this comment.
Thanks! I am like 80% sure the islice() is wrong. There are also a fair amount of test failures, I didn't go through them (slooow internet in the train), but they might be related
|
I think the failures are something different, because, the opensuse/centos pairing notwithstanding, they're all Debian. I've only quickly looked at debian/debian, but there it's which is a message from |
martinpitt
left a comment
There was a problem hiding this comment.
Thanks! I cannot resolve my own threads, but all good now.
behrmann
left a comment
There was a problem hiding this comment.
One last thing: Could you rebase against main so that we can run CI again? There was a change in Debian that broke CI (unrelated to your change).
e1c537f to
b38e981
Compare
The original guard when building the new todo is: ``` todo += [m for m in depinfo.modules if m not in mods and m in nametofile] ``` `mods` only accumulates modules from previous iterations of the outer while loop. Within a single iteration, modules in the current batch are added to mods one at a time as `moddep.items()` is iterated. This means two distinct failure modes: 1. Cross-dependency within a batch: if modules A and B are both in the current todo and both depend on X, X passes the m not in mods check when processing A's deps and again when processing B's (since X hasn't been added to mods yet). X ends up in todo twice. 2. Intra-batch back-edge: if A depends on B and both are already in the current todo/moddep, B still passes m not in mods when processing A's dep list, queuing B for a redundant second modinfo call in the next iteration. Both boil down to the same root cause: the check doesn't account for modules currently being processed. The fix addresses both issues: 1. `m not in moddep.keys()` prevents modules from the current batch being re-queued into the next iteration's `todo`. 2. Turning`todo` into a set prevents a module from appearing *multiple times* in the same next `todo`, when two modules in the current batch share a dependency Because these duplicates frequently occur in practice, eliminating them provides an incremental speedup on top of the original optimization made in GH4092/GH4017. Co-Authored-By: Jörg Behrmann <behrmann@physik.fu-berlin.de>
b38e981 to
45dd045
Compare
|
Thanks @BroadlyWhitaker! |
The original guard when building the new todo is:
modsonly accumulates modules from previous iterations of the outer while loop. Within a single iteration, modules in the current batch are added to mods one at a time asmoddep.items()is iterated. This means two distinct failure modes:Cross-dependency within a batch: if modules A and B are both in the current todo and both depend on X, X passes the m not in mods check when processing A's deps and again when processing B's (since X hasn't been added to mods yet). X ends up in todo twice.
Intra-batch back-edge: if A depends on B and both are already in the current todo/moddep, B still passes m not in mods when processing A's dep list, queuing B for a redundant second modinfo call in the next iteration.
Both boil down to the same root cause: the check doesn't account for modules currently being processed.
The fix addresses both issues:
m not in moddep.keys()prevents modules from the current batch being re-queued into the next iteration'stodo.Turning
todointo a set prevents a module from appearing multiple times in the same nexttodo, when two modules in the current batch share a dependencyBecause these duplicates frequently occur in practice, eliminating them provides an incremental speedup on top of the original optimization made in GH4092/GH4017.