GNOME Bugzilla – Bug 750168
Reduce the amount of copying of partition objects
Last modified: 2015-08-03 17:30:59 UTC
Currently GParted copies partition objects all over the place. This enhancement will reduce that copying to approximately only when needed because the partition object is going to be changed or has a separate life time. The primary reason is as a preparatory step in the implementation of LUKS support. Bug 627701 - option to encrypt new partitions (using LUKS). My current implementation strategy is to create a PartitionLUKS class derived from the Partition class. This implies polymorphism of Partition objects, which in C++ requires using pointers and references to objects, and not using objects directly. (See C++ object slicing). Secondary reason is that it is just inefficient to be copying partition objects and constructing them all over the place. Many members are themselves objects which need constructing such as those of type Glib::string and vectors of Glib::string. This is especially bad when the partition is an extended partition and contains a vector of logical partitions which have to be copied too.
Created attachment 304548 [details] [review] Copy partition objects less (v1) Hi Curtis, Here's patchset v1 for this. All the copying partition objects less changes ended up mostly just being in the GUI. Working out this patchset is the first time I actually understood how the core of the GParted GUI works. Result is in patch: Document how the GUI works and the lifetimes of important data And there is this inefficiency in the GUI. Each time Refresh_Visual() is run (because a new operation has been queued, switch to a new device or Ctrl-R pressed) it re-copies the current partitions and visually re-applies all queued operations. Thanks, Mike
Created attachment 304550 [details] [review] Partition object copying (DEBUG 1) DO NOT APPLY UPSTREAM In case you're interested try applying this patchset and compare the amount of partition object copying going on before and after. Especially when just clicking to select partitions.
Hi Mike, It looks like you've been quite busy. And congrats on figuring out the GParted GUI. :-) Since the patch set in comment #1 is a fair size, I anticipate it will take a few days for me to come up to speed and properly review. > And there is this inefficiency in the GUI. Each time Refresh_Visual() > is run (because a new operation has been queued, switch to a new device > or Ctrl-R pressed) it re-copies the current partitions and visually > re-applies all queued operations. Off the top of my head, there might be at least one case where it makes sense to visually re-apply all queued operations. I'm thinking of the case where a new operation is added, but GParted determines that it can merge the operation with a previously queued operation. In this case one of the previously queued operations would be changed. For example, two or more resize operations on the same partition that are merged by Win_GParted::Merge_Operations(). Curtis
Hi Curtis, > > And there is this inefficiency in the GUI. Each time Refresh_Visual() > > is run (because a new operation has been queued, switch to a new device > > or Ctrl-R pressed) it re-copies the current partitions and visually > > re-applies all queued operations. > > Off the top of my head, there might be at least one case where it makes > sense to visually re-apply all queued operations. I'm thinking of the case > where a new operation is added, but GParted determines that it can merge the > operation with a previously queued operation. In this case one of the > previously queued operations would be changed. For example, two or more > resize operations on the same partition that are merged by > Win_GParted::Merge_Operations(). You've identified the hardest update operations and redraw case. These are the 3 types of cases as I see them: 1) Add new operation; 2) Remove operation; 3) Merge new operation with earlier operation. At the moment GParted just updates the vector of pending operations and then does the previously mentioned; copy current partitions as on the disk and visually re-apply all queued operations. This works in all 3 cases and is simple. It costs a bit of CPU time and does some partition object copying re-appling all those operations each time. However simple it good. It is easy to understand. I will NOT be changing this. Perhaps inefficient was the wrong adjective. > Since the patch set in comment #1 is a fair size, I anticipate it will take a > few days for me to come up to speed and properly review. Take your time. It's taken me since just after Easter of various experiements to get this far. Even if future code changes for LUKS support takes a different direction I think that these changes are worth keeping and hopefully don't make the code any more complicated to understand. Mike
Hi Mike, > However simple it good. It is easy to understand. I completely agree. We've some warm weather forecast for this weekend so I'm hoping to spend some more time on my bicycle and somewhat less on the computer. :-) Curtis
Hi Mike, My review of patch set v1 in comment #2 is going well. In particular I like the change in the last patch to ensure that Set_Data() is always invoked appropriately instead of relying on the programmer having to figure this out. If you don't mind, I plan to make three minor git comment changes: 1) Repeated word "the" in P1: Move vector of partition objects to a Win_GParted class member FROM: This will allow for the the above cases ... TO: This will allow for the above cases ... 2) Change to word tense "introduce" in P6: Assert selected_partition_ptr is not NULL FROM: ... ensure that a bug doesn't get introduces which allows a ... TO: ... ensure that a bug doesn't get introduced which allows a... 3) Minor grammar issue in P10: Stop copying selected partition object in Information dialog FROM: ... dereferences a pointer to an object in the context of a ^ ^ needing a reference to the object doesn't copy the object. TO: ... dereferences of a pointer to an object in the context of ^^ ^^ needing a reference to the object doesn't copy the object. With the code review complete, I will move onto regression testing of basic GParted functionality. Curtis
Thank you Mike for this patch set and your work towards LVM encrypted volume support. Patch set v1 in comment #1 has passed all my regression tests. For testing I used both ntfs and ext4 file systems in an openSUSE 13.1 virtual machine. I tested different combinations of create, grow, shrink, move, copy, check, label, and new UUID, including pasting into existing partitions, and all operations completed as expected. I also performed some random tests on kubuntu 12.04 and fedora 22, including manage flags, and all finished with the anticipated outcome. As such I have committed patch set v1 from comment #1 (including the minor git comment changes mentioned in comment #6) to the git repository for inclusion in the next release of GParted. The relevant git commits can be viewed at the following links: Move vector of partition objects to a Win_GParted class member (#750168) https://git.gnome.org/browse/gparted/commit/?id=545b75d9574e053cadb9ddd5cf15fc28181da87c Pass by pointer in the signal_partition_selected callbacks (#750168) https://git.gnome.org/browse/gparted/commit/?id=c430acf52affb07d65cad78f598e31fb5f1c05ee Store pointers to partition objects in TreeView_Details (#750168) https://git.gnome.org/browse/gparted/commit/?id=acd5d7e580007894147e1dedb3a2d8e0f80e7477 Store pointers to partition objects in DrawingAreaVisualDisk (#750168) https://git.gnome.org/browse/gparted/commit/?id=cc1448abd237291e1775aa88b67faa47890a5450 Change selected partition into a pointer (#750168) https://git.gnome.org/browse/gparted/commit/?id=da39e3cad34b1d8fb50a2d0321bba832fd1d38d3 Assert selected_partition_ptr is not NULL (#750168) https://git.gnome.org/browse/gparted/commit/?id=5e027d6989f1423026d6c8b49b2237cd286a40af Assert selected_partition_ptr is valid before use (#750168) https://git.gnome.org/browse/gparted/commit/?id=6ae327c8f98ecd954393c47bce25cb987b14e351 Document how the GUI works and the lifetimes of important data (#750168) https://git.gnome.org/browse/gparted/commit/?id=1143917d342cf701f524b0779fa06f20634f921a Stop copying displayed partition objects in activate_resize() (#750168) https://git.gnome.org/browse/gparted/commit/?id=7f05037dbc99e957d6ec15499eeb42e6786605f9 Stop copying selected partition object in Information dialog (#750168) https://git.gnome.org/browse/gparted/commit/?id=efaea943014827cbda43412fb6c2081b4491ef19 Stop copying selected partition object in Manage Flags dialog (#750168) https://git.gnome.org/browse/gparted/commit/?id=ece945685ca71c0adef42f3e4bd71c8ba6e381e1 Shallow copy Device object into Operation object (#750168) https://git.gnome.org/browse/gparted/commit/?id=90e3ed68fc60c1395c2f3c20ed0b4493037753fb Stop copying selected_partition back on itself in the copy dialog (#750168) https://git.gnome.org/browse/gparted/commit/?id=8b96f8409f9fe1777a1283c11ca7c58c229be2df Rename Dialog_Base_Partition member to new_partition https://git.gnome.org/browse/gparted/commit/?id=32a5ace1561f7b304129ab5a06e4f4c57e0af5e6 Remove Set_Data() from the copy, resize/move and new dialog class APIs https://git.gnome.org/browse/gparted/commit/?id=7a4a375ed629fea77995c98d13bd1992231be6fb
This enhancement was included in the GParted 0.23.0 release on August 3, 2015.