merging events is slow
Submitted by Adam Dingle
Link to original bug (#715584)
Description
---- Reported by adam@yorba.org 2009-11-03 11:06:00 -0800 ----
Original Redmine bug id: 979
Original URL: http://redmine.yorba.org/issues/979
Searchable id: yorba-bug-979
Original author: Adam Dingle
Original description:
merging events is slow
Related issues:
- related to shotwell - 6057: Moving tags in the hierarchy takes a looong time (Open)
---- Additional Comments From shotwell-maint@gnome.bugs 2013-05-16 14:45:00 -0700 ----
History
Comment 1
Updated by Adam Dingle almost 4 years ago
- Target version set to 0.4
Comment 2
Updated by Adam Dingle almost 4 years ago
-
Target version deleted (
<strike>
_0.4_</strike>
)
Seems good enough for 0.4 at least. Dropping from this milestone.
Comment 3
Updated by Adam Dingle almost 4 years ago
-
Priority deleted (
<strike>
_High_</strike>
)
Comment 4
Updated by Jim Nelson about 3 years ago
Comment 5
Updated by Jeff Fortin almost 2 years ago
I noticed that shotwell flickers a LOT during a merge, because it actually shows you the current meta-album with stuff being merged into it (or out of it?). This is most likely one of the big performance problems you have: you're doing needless work.
Don't show me the whole stuff in the background, just show a progressbar. Then update the UI when you're done. The flickering is distracting and probably taxes the GPU and CPU a lot.
Comment 6
Updated by Tim Koopman over 1 year ago
I just merged 1600 photos and it took 4 minutes. Why does this take so long? This should be done almost instantly.
Comment 7
Updated by Adam Dingle over 1 year ago
- Priority changed from Low to Normal
Comment 8
Updated by komatsu - over 1 year ago
Merging is lot of quicker when you change the view out of affected events during the merge. The redrawing is the main performance hog.
Comment 9
Updated by Jim Nelson 11 months ago
- Target version set to 0.14.0
Comment 10
Updated by Jim Nelson 11 months ago
- Category set to ux
Comment 11
Updated by Jim Nelson 11 months ago
- Category changed from ux to performance
Comment 12
Updated by Arthur Lutz 10 months ago
Jeff Fortin wrote:
I noticed that shotwell flickers a LOT during a merge, because it actually shows you the current meta-album with stuff being merged into it (or out of it?). This is most likely one of the big performance problems you have: you're doing needless work.
Don't show me the whole stuff in the background, just show a progressbar. Then update the UI when you're done. The flickering is distracting and probably taxes the GPU and CPU a lot.
+1 (same applies to "New Event")
Comment 13
Updated by Jim Nelson 10 months ago
- Target version changed from 0.14.0 to 0.15.0
Comment 14
Updated by Joe Bylund 8 months ago
Was there a change that fixed this? I no longer see the flickering during event merge, and it's much faster.
Comment 15
Updated by Jim Nelson 8 months ago
It's quite likely something has changed in the interim that fixes this problem. I'd like for us to do some stress testing and verify this is fixed via various use cases before closing, however.
Comment 16
Updated by komatsu - 8 months ago
I can still see the problem on 14.1. A new event with 23 photos took like a 7s on my Lenovo X201i machine. Event merging looks the same.
Comment 17
Updated by Lucas Beeler 8 months ago
- Keywords set to needs-testing
Added needs-testing keyword and bumped to high priority since there are conflicting reports from the wild about whether this issue has been fixed or not.
Comment 18
Updated by Lucas Beeler 8 months ago
- Priority changed from Normal to High
Comment 19
Updated by Arthur Lutz 8 months ago
komatsu - wrote:
I can still see the problem on 14.1. A new event with 23 photos took like a 7s on my Lenovo X201i machine. Event merging looks the same.
Still slow for me too after an upgrade to 14.1 on ubuntu.
I'd be fine with the slowness of the operation if it was done in the background, should the focus not be on making this a "background" task ?
Comment 20
Updated by Clinton Rogers 8 months ago
One thing I've noticed is that, when moving images around between events, a
LOT of calls to Photo.get_exposure_time() occur; in fact, it looks like it
gets called once on every image in the library for any event move at all
(including images that aren't being worked on at the time.
It doesn't seem like this is a terribly expensive call, but with a large
library, I could see how these could add up and cause significant amounts
of lag.
Comment 21
Updated by Lucas Beeler 7 months ago
-
Keywords deleted (
<strike>
_needs-testing_</strike>
)
Removed the needs-testing keyword. This problem definitely still occurs and the time to merge events appears proportional to library size.
Comment 22
Updated by Lucas Beeler 7 months ago
- Assignee set to Clinton Rogers
Special recognition goes to Clint for figuring out via gprof that the event merge operation requires quering the exposure date/time for every photo in the user's library, not merely those involved in the merge.
Comment 23
Updated by Clinton Rogers 7 months ago
- Status changed from Open to Review
- % Done changed from 0 to 20
Please look at [origin/bug/979-speed-up-event-merging-by-caching-exposure-time SHA a245d149243c9b32a0a97c5f36e3f93a5d059fe1](http://redmine.yorba.org/project s/shotwell/repository/revisions/a245d149243c9b32a0a97c5f36e3f93a5d059fe1) for this (it's part 1 of a multipronged attack).
Merging 5000-photo event of mixed dates with two 1-photo events with 8.5k library:
*Before patch:* ~67 sec.
*After patch:*  _place_holder;~44 sec.
In addition, this should have a slight beneficial effect anywhere
get_exposure_time()
gets called on a large view collection of images, such
as when switching between checkerboard pages.
Comment 24
Updated by Clinton Rogers 7 months ago
One other possible optimization I just spotted is to make the event with the most photos the 'master' event in the command (currently, it's the first in the list, which means that, if you're merging a 1000-photo event into a 1-photo one, it'll naively and blithely move the thousand photos, rather than just moving the one).
Comment 25
Updated by Lucas Beeler 7 months ago
- Status changed from Review to Open
@Clint: for this iteration (git rev a245d149) -- which I'm assuming will be the first of several -- unless I'm missing something, why do we need the get_exposure_time_from_row( ) method? It's not called from anywhere. If we do need this, come talk to me about it. Otherwise remove it. Also, at Photo.vala:456, there's no reason to lock this.row. Since the object won't even exist until its constructor finishes execution, there's no risk of the object being accessed from multiple threads. Make these changes, do some smoke testing, and send me an updated diff!
Comment 26
Updated by Clinton Rogers 7 months ago
e03e0932 is the exposure date/time caching thing pushed into master and should improve speed a bit; more to come.
Comment 27
Updated by Clinton Rogers 7 months ago
- Status changed from Open to Review
- % Done changed from 20 to 40
Please see 3d6d670b25a3d82a95db5eedac295fec38d65efd - it implements the optimization from comment 24. The actual amount of speedup varies with the number of images in play, but if the two events' sizes are sufficiently disparate, it can be noticeable (on the order of multiple seconds).
Comment 28
Updated by Lucas Beeler 7 months ago
- Status changed from Review to Open
Review: Let's discuss. Clint, unless I'm missing something, doesn't your code allow for an unnamed event to become the master event even if there exists at least one event with a name? That's not what should happen here. Names are user-defined information and we should try to preserve them, even if we have to take a slight performance hit. So I'd prefer that it not be possible for an unnamed event to become the master unless all events are unnamed, in which case the master should be largest event.
Comment 29
Updated by Clinton Rogers 7 months ago
- Status changed from Open to Review
An updated patch is available; please see 9a5970a50d902b22306c31b64d3ef30c4168177e .
Comment 30
Updated by Lucas Beeler 7 months ago
Review: Approve. Commit the revised version!
Comment 31
Updated by Clinton Rogers 7 months ago
- Status changed from Review to Open
- % Done changed from 40 to 50
Per Lucas, pushing cdeeef0a, but setting back to open (still more to come).
Comment 32
Updated by Clinton Rogers 7 months ago
It seems as if the bulk of the problem is the SortedList lurking behind the scenes; it appears that the list gets resorted once for every photo that's moved into and out of an event...
Comment 33
Updated by Clinton Rogers 7 months ago
- % Done changed from 50 to 70
Please review b9f46f26ac2552ae6dfa323f3c10d1ed485e1f37 for this.
It tries to address the problem outlined in comment 32 below.
Comment 34
Updated by Clinton Rogers 7 months ago
- Status changed from Open to Review
Comment 35
Updated by Lucas Beeler 7 months ago
- Status changed from Review to Open
Review: not approved. Clint, please re-work this patch along the lines that Jim, you, and I just discussed.
Comment 36
Updated by Clinton Rogers 7 months ago
- Status changed from Open to Review
Please look to 5316448cd48cf1c969ee87061a0d04f209f600e3 for this.
Comment 37
Updated by Clinton Rogers 7 months ago
If it still isn't fast enough after this, perhaps we could use the same trickery when detaching as well.
Comment 38
Updated by Lucas Beeler 7 months ago
- Status changed from Review to Open
Clint and I discussed this yesterday and there's more work to be done. Setting the ticket back to "Open."
Comment 39
Updated by Clinton Rogers 7 months ago
- Status changed from Open to Review
Please look to 1da10dc2c4bbefa3a5b2c399382451be486ee02a for this.
Repeating the test below yields about 36 seconds.
Comment 40
Updated by Joe Bylund 7 months ago
Just a question: is the same cost paid when creating a new event?
Comment 41
Updated by Clinton Rogers 7 months ago
940d98d5a1589150def43ef6ab7b684ab212982a should bring this up-to-date for reviewing.
@Joseph Bylund: I haven't examined it, although I would expect so (it certainly 'feels' like it). If this is accepted, it may be helpful to take the same approach there, too.
One interesting clue that leads me to believe that yes, we're paying for far too many list sorts when making a new event is the fact that making a new event with, say, 1000 undated images is both measurably and perceptually far faster than making a new event with 1000 photos whose EXIF fields contain proper date/time data.
Comment 42
Updated by Lucas Beeler 7 months ago
- Status changed from Review to Open
Review: needs work. Make the set/attach operation atomic, as discussed.
Comment 43
Updated by Jim Nelson 6 months ago
-
Target version deleted (
<strike>
_0.15.0_</strike>
)
Comment 44
Updated by Jim Nelson 6 months ago
-
Assignee deleted (
<strike>
_Clinton Rogers_</strike>
)
--- Bug imported by chaz@yorba.org 2013-11-25 21:42 UTC ---
This bug was previously known as bug 979 at http://redmine.yorba.org/show_bug.cgi?id=979
Unknown version " in product shotwell. Setting version to "!unspecified". Unknown milestone "unknown in product shotwell. Setting to default milestone for this product, "---". Setting qa contact to the default for this product. This bug either had no qa contact or an invalid one. Resolution set on an open status. Dropping resolution
Resolution: RESOLVED DUPLICATE