GNOME Bugzilla – Bug 750582
Refactor the LVM2_PV_Info module object interface and internal cache representation
Last modified: 2015-08-03 17:31:18 UTC
The interface and working of the LVM2_PV_Info cache module looks clunky after the approach I took with the btrfs device cache. It caches lines of text and splits / parses them every time they are accessed. The class API has a multi-object interface yet is nothing like that. Refactor the LVM2_PV_Info module to parse the data when loaded and store a structure of values of the correct type in the cache. Also get rid of the multi-object interface. Patches to follow. Thanks, Mike
Created attachment 304799 [details] [review] Refactor LVM2_PV_Info module (v1) Hi Curtis, Here's the patchset for this. In terms of testing I created the LVM setup outlined in the data model examples in the comment near the top of the LVM2_PV_Info.cc file. I then added print statements into all the public members and checked the return values extracted from the cache were the same before and after this patchset. They were identical, except for the delayed loading of the cache until first use. Also here is the timing data for my statement about saving 1.0 seconds from 3.7 seconds if there are no LVM2 PVs so no LVM2 querying command need running and the cache loading: 15.623306 +11.797657 STOPWATCH_RESET 0.000000 +0.000000 set_devices_thread() (pdevices) ... 1.374063 +0.038028 execute_command() execute: lvm vgscan 1.770824 +0.396761 execute_command() exit status 0 1.770879 +0.000055 execute_command() execute: lvm pvs --config "log{command_names=0}" --nosuffix --noheadings --separator , --units b -o pv_name,pv_size,pv_free,vg_name 2.096226 +0.325347 execute_command() exit status 0 2.096288 +0.000062 execute_command() execute: lvm pvs --config "log{command_names=0}" --nosuffix --noheadings --separator , --units b -o vg_name,vg_attr,lv_name,lv_attr 2.420457 +0.324169 execute_command() exit status 0 ... 3.734295 +0.002948 set_devices_thread() return 0.396761 + 0.325347 + 0.324169 = 1.046277 seconds Thanks, Mike
Created attachment 304901 [details] [review] Refactor LVM2_PV_Info module (v2) Hi Curtis, Here's v2 of the patchset. Only difference is an easy rebase to apply on top of bug 750168, patchset v1 (attachment #304548 [details] "Copy partition objects less (v1)"). Thanks, Mike
Thank you Mike for these LVM2_PV_Info class improvements. If I understand patch set v2 (from comment #2) correctly, it looks like the cache loading logic was changed. Instead of indicating in GParted when to (re)load the cache, a person now indicates when to clear the cache. The class will automatically load the cache if needed. Also the LVM2_PV_Info class is treated more like the already existing Utils class which is a container of methods (e.g., call the class method directly instead of creating instance of class and then invoking methods). These changes look good to me. :-) My regression testing in kubuntu 12.04 and fedora 22 did not find any changes in behaviour from a user perspective. As such this patch set is good to go. I made the following minor changes to git comments in the following patches: P1: Stop borrowing the constructor to load the LVM2_PV_Info cache FROM: The C++ compiled will provide ... TO: The C++ compiler will provide ... ^ P3: Parse LVM2_PV_Info cache into fields while loading FROM: GParted use to cache the results ... TO: GParted used to cache the results ... ^ Patch set v2 has been committed to the git repository for inclusion in the next release of GParted. The relevant git commits can be viewed at the following links: Stop borrowing the constructor to load the LVM2_PV_Info cache (#750582) https://git.gnome.org/browse/gparted/commit/?id=81f0b934bce9a8243735e2b8d05c6d70d1e109be Stop needing any LVM2_PV_Info objects (#750582) https://git.gnome.org/browse/gparted/commit/?id=2c5e7b0d90d089858b063632d9b2eafcc17b6cbd Parse LVM2_PV_Info cache into fields while loading (#750582) https://git.gnome.org/browse/gparted/commit/?id=e6f7ea01f92266c764b8d1fe95c996ff57cdda02 Delay loading LVM2_PV_info cache until actually needed (#750582) https://git.gnome.org/browse/gparted/commit/?id=9be8d3760003a4bc726e37e42a99c2dd921263d6 Provide comment for btrfs::clear_cache() call https://git.gnome.org/browse/gparted/commit/?id=d405bb22645ce72a235d3ad3346b9cc6a4f0c9f9
This enhancement was included in the GParted 0.23.0 release on August 3, 2015.