GNOME Bugzilla – Bug 590059
GnomeRR: Use hwdb for pnp.ids
Last modified: 2016-06-25 11:31:22 UTC
pnp.ids is 50kB, so that's roughly what is needed to keep that in memory all the time (at least for gnome-settings-daemon). Not sure it's worth the effort to try to fix this...
Is it worth making this a hash table? It's small enough (and rarely-used enough) that a straight linear search would probably be just fine, which opens the door for read-only mmap.
Fine with me too ;-)
Created attachment 216854 [details] [review] pnp-ids: Use an mmap'ed file instead of a HashTable Which means we don't need the object to be a singleton, and saves us about 50kB of data most of the time we don't look at it.
Something like that? You can test it with test-pnp-ids in the same directory. It's much slower than the hash table version (as expected), 500k searches take nearly 3 minutes instead of 0.1 second. Not sure that's awfully problematic though.
Do we actually have multiple consumers of this data ? Otherwise, the mmap won't actually help us much, right ? Could use either gperf or gvdb to get the best of both worlds (mmap + O(1) lookup), I guess ? Or make the fields fixed-width, sort the list and use bsearch. But all of that can be done later, if this approach turns out to be a problem.
Review of attachment 216854 [details] [review]: Looks fine to me. ::: libgnome-desktop/gnome-pnp-ids.c @@ +204,1 @@ /* parse into lines */ This comment is now slightly misleading.
(In reply to comment #5) > Do we actually have multiple consumers of this data ? Otherwise, the mmap won't > actually help us much, right ? All users of gnome-desktop in gnome-settings-daemon, and the color plugin. Might worth checking whether it affects http://git.gnome.org/browse/gnome-settings-daemon/commit/gnome-settings-daemon/gnome-settings-manager.c?id=d8c158e74caea048dea5a38232bd87c1c7a78a70 adversely or not. > Could use either gperf or gvdb to get the best of both worlds (mmap + O(1) > lookup), I guess ? Or make the fields fixed-width, sort the list and use > bsearch. I don't know whether the tear-down and build-up is the expensive part we're trying to avoid. I can't think that the lookup is something we'd do extensively enough that it would make a difference. > But all of that can be done later, if this approach turns out to be a problem.
(In reply to comment #7) > All users of gnome-desktop in gnome-settings-daemon, and the color plugin. > > Might worth checking whether it affects > http://git.gnome.org/browse/gnome-settings-daemon/commit/gnome-settings-daemon/gnome-settings-manager.c?id=d8c158e74caea048dea5a38232bd87c1c7a78a70 > adversely or not. That commit mentions saving 400ms at startup, which is totally worthwhile. It would be interesting to see why that process is so slow (even if it's done three times, that's 133 msec per iteration - plenty enough to seek, read, and parse a small file, unless it's doing something really dumb). Can someone grep through the code and make a list of the places where the PNP ids are used? Then we can see how much time we can spend in each place, and whether we can just allocate memory for the table temporarily or whether it's better to keep it around in memory for a while.
Putting on NEEDINFO so people do real benchmarking of the effects of this patch on startup time.
We should use udev's hwdb, but we'll need to wait for it being split out before that happens.
*** Bug 688848 has been marked as a duplicate of this bug. ***
Created attachment 303652 [details] [review] pnp-ids: Use udev's hwdb to query PNP IDs hwdb has its own database which it compiles to binary, so that: 1) initialisation is practically free See https://git.gnome.org/browse/gnome-settings-daemon/commit/gnome-settings-daemon/gnome-settings-manager.c?id=d8c158e74caea048dea5a38232bd87c1c7a78a70 2) lookups are also quick, as they use a binary DB 3) we don't have to maintain our own copy of the database The only problem being that non-Linux OSes will need reimplement this section of the code. This will be on top of the usual downstream patches they ship for a number of components.
Last time I spoke to Kay he was against this; probably worth asking him his views now.
(In reply to Richard Hughes from comment #13) > Last time I spoke to Kay he was against this; probably worth asking him his > views now. "this" being making hwdb available on non-Linux? I'm hoping that this will be another patch that non-Linux OSes can ship. Antoine?
(In reply to Bastien Nocera from comment #14) > (In reply to Richard Hughes from comment #13) > > Last time I spoke to Kay he was against this; probably worth asking him his > > views now. > > "this" being making hwdb available on non-Linux? I'm hoping that this will > be another patch that non-Linux OSes can ship. > > Antoine? Hi Bastien. Thanks for putting me into the loop. If it's just a matter of reverting the commit; then sure. If we need a new code implementation, I'll have to have a look at the actual code which I cannot do right now. I must admit I don't even know what it is used by...
(In reply to Antoine Jacoutot from comment #15) > (In reply to Bastien Nocera from comment #14) > > (In reply to Richard Hughes from comment #13) > > > Last time I spoke to Kay he was against this; probably worth asking him his > > > views now. > > > > "this" being making hwdb available on non-Linux? I'm hoping that this will > > be another patch that non-Linux OSes can ship. > > > > Antoine? > > Hi Bastien. Thanks for putting me into the loop. > > If it's just a matter of reverting the commit; then sure. OK. > If we need a new code implementation, I'll have to have a look at the actual > code which I cannot do right now. I must admit I don't even know what it is > used by... It's used by gnome-settings-daemon, and gnome-control-center, to get a vendor name from the EDID.
(In reply to Bastien Nocera from comment #14) > "this" being making hwdb available on non-Linux? I'm hoping that this will > be another patch that non-Linux OSes can ship. Sorry, no, "this" as in using the hwdb like this. I'll ping Kay.
Sorry, nothing changed from out side, we have no interest in maintaining systemd or any of its components for other kernels. The hwdb stuff is pretty much Linux-focussed, as the matches are basically just kmod "modalias" matches. This is not going to change, sorry. If GNOME wants to support other kernels it needs to make support for this functionality optional, remove support for it, or maintain its own ported copy of it...
(In reply to Lennart Poettering from comment #18) > Sorry, nothing changed from out side, we have no interest in maintaining > systemd or any of its components for other kernels. The hwdb stuff is pretty > much Linux-focussed, as the matches are basically just kmod "modalias" > matches. This is not going to change, sorry. > > If GNOME wants to support other kernels it needs to make support for this > functionality optional, remove support for it, or maintain its own ported > copy of it... That's not the question that was asked...
(In reply to Lennart Poettering from comment #18) > Sorry, nothing changed from out side, we have no interest in maintaining > systemd or any of its components for other kernels. The hwdb stuff is pretty > much Linux-focussed, as the matches are basically just kmod "modalias" > matches. This is not going to change, sorry. > > If GNOME wants to support other kernels it needs to make support for this > functionality optional, remove support for it, or maintain its own ported > copy of it... I'll ask again. We want to know if it's ok to use hwdb as we did in the patch, not whether you want to port it to non-Linux.
Sorry for not reviewing this more timely. Using the hwdb for this certainly makes sense. It's a weirdly named of course for this purpose, given that a display isn't an ACPI device, but the EISA, EDID, ACPI, ISAPNP and UEFI ids all use the same namespace, hence reusing this here makes sense. The hwdb lookup in the patch doesn't look right however. The key you look up should be generated as "acpi:" + $PNPID + ":" (that means: do not include an asterisk in the string, as you currently do). And use the full $PNPID you got from the display, unmodified and not truncated. Then, iterate through the list you get back and look for the ID_VENDOR_FROM_DATABASE property to acquire the friendly vendor string. You may also use ID_MODEL_FROM_DATABASE if you like, to acquire a friendly model string for the device. But note that currently the acpi database doesn't actually set that for any device. (It sets it for USB and PCI though, and hence if somebody contributes model strings for the PNP ids, too it would show up under that property.) you can test the lookup btw with this: udevadm hwdb -t acpi:LNXCPU: It will output you the database entries it finds. (of course, use your display's PNPID here, instead of "LNXCPU", to make this a better example)
I added the following lines /usr/lib/udev/hwdb.d/20-acpi-vendor.hwdb: acpi:DEL4085: ID_VENDOR_FROM_DATABASE=Dell Inc. ID_MODEL_FROM_DATABASE=Dell S2340T (that's my monitor) And ran: sudo udevadm hwdb --update I ran, to test the database: $ udevadm hwdb -t acpi:DEL4085: ID_VENDOR_FROM_DATABASE=Dell Inc. ID_MODEL_FROM_DATABASE=Dell S2340T You can use those changes to test the changes I made to the patch.
Created attachment 330352 [details] [review] pnp-ids: Use udev's hwdb to query PNP IDs hwdb has its own database which it compiles to binary, so that: 1) initialisation is practically free See https://git.gnome.org/browse/gnome-settings-daemon/commit/gnome-settings-daemon/gnome-settings-manager.c?id=d8c158e74caea048dea5a38232bd87c1c7a78a70 2) lookups are also quick, as they use a binary DB 3) we don't have to maintain our own copy of the database 4) allows model lookup rather than simply vendor lookup, when the database contains that information The only problem being that non-Linux OSes will need reimplement this section of the code. This will be on top of the usual downstream patches they ship for a number of components.
Created attachment 330353 [details] [review] pnp-ids: Use udev's hwdb to query PNP IDs hwdb has its own database which it compiles to binary, so that: 1) initialisation is practically free See https://git.gnome.org/browse/gnome-settings-daemon/commit/gnome-settings-daemon/gnome-settings-manager.c?id=d8c158e74caea048dea5a38232bd87c1c7a78a70 2) lookups are also quick, as they use a binary DB 3) we don't have to maintain our own copy of the database 4) allows model lookup rather than simply vendor lookup, when the database contains that information The only problem being that non-Linux OSes will need reimplement this section of the code. This will be on top of the usual downstream patches they ship for a number of components.
Attachment 330353 [details] pushed as b8cbfbe - pnp-ids: Use udev's hwdb to query PNP IDs