[UDevManager] Already filter for subsystem before querying
ClosedPublic

Authored by broulik on Mar 13 2018, 3:12 PM.

Details

Summary

This does some rough filtering in advance before creating our wrapper items with properties and all.
UDev default 70-uaccess.rules assumes, ID_MEDIA_PLAYER is always in "usb" subsystem

CCBUG: 391738

Test Plan

Gives a significant speedup of Dolphin startup for me. Instead of KFilePlacesModel spending 120ms querying for Udev devices it only takes 30ms.

My phone still shows up in Dolphin and unplugging and plugging it back in also works.

Diff Detail

Repository
R245 Solid
Lint
Lint Skipped
Unit
Unit Tests Skipped
broulik created this revision.Mar 13 2018, 3:12 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 13 2018, 3:12 PM
broulik requested review of this revision.Mar 13 2018, 3:12 PM

It brings the number of sys calls from 28000 down to 860 here using the first grep command in the comment, could have helped somewhat I guess but the second command is completely bogus for me. needs further investigation. It does give a significant speedup which is already worth it ;)

broulik edited the summary of this revision. (Show Details)Mar 13 2018, 3:20 PM

Cool! Any chance we can add an autotest? Solid's code seems somewhat fragile, judging by the regressions that were accidentally introduced in https://cgit.kde.org/solid.git/commit/?id=1384f275ab2f1ad1841753ee163af6d1b0bb952b

Copying our IRC chat:

I tried something /very very/ similar https://phabricator.kde.org/D8495?vs=on&id=21368&whitespace=ignore-most#toc which merges queryDeviceInterface into the earlier query. It didn't work.

This patch is a bit different, it does a coarse filter, then still goes through queryDeviceInterface afterwards.

My concern with this is that your filter query for PortableMediaPlayer doesn't match what queryDeviceInterface(PortableMediaPlayer) does.
That code does "devicesByProperty("ID_MEDIA_PLAYER", QVariant()"

If you can prove that property is only valid for USB subsystems, then you can ship it, otherwise, no.

If you can prove that property is only valid for USB subsystems, then you can ship it, otherwise, no.

From udev code https://github.com/systemd/systemd/blob/master/src/login/70-uaccess.rules#L72

SUBSYSTEM=="usb", ENV{ID_MEDIA_PLAYER}=="?*", TAG+="uaccess"
broulik updated this revision to Diff 29416.Mar 13 2018, 3:57 PM
broulik edited the summary of this revision. (Show Details)
  • Check for usb subsystem also in queryDeviceInterface
davidedmundson accepted this revision.Mar 13 2018, 3:58 PM
This revision is now accepted and ready to land.Mar 13 2018, 3:58 PM
This revision was automatically updated to reflect the committed changes.
mpyne added a subscriber: mpyne.Mar 13 2018, 11:36 PM

Any reason not to close the bug 391738? I can do so but wanted to make sure it wasn't intentionally left open.

I wasn't sure if that really fixes it, it might mitigate it? If you could perhaps retry with your strace skills. If you consider it good enough, feel free to close the bug

mpyne added a comment.Mar 15 2018, 1:07 AM

I never had the bug myself, I just grepped through the attached log to see if it made sense or not. :) I'll close the bug for now, if it doesn't fix the issue then it'll just get reopened.