---- 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

####

#1

Updated by Adam Dingle almost 4 years ago

  * **Target version** set to _0.4_

####

#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.

####

#3

Updated by Adam Dingle almost 4 years ago

  * **Priority** deleted (<strike>_High_</strike>)

####

#4

Updated by Jim Nelson about 3 years ago

  1. # is a duplicate of this.

####

#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.

####

#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.

####

#7

Updated by Adam Dingle over 1 year ago

  * **Priority** changed from _Low_ to _Normal_

####

#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.

####

#9

Updated by Jim Nelson 11 months ago

  * **Target version** set to _0.14.0_

####

#10

Updated by Jim Nelson 11 months ago

  * **Category** set to _ux_

####

#11

Updated by Jim Nelson 11 months ago

  * **Category** changed from _ux_ to _performance_

####

#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")

####

#13

Updated by Jim Nelson 10 months ago

  * **Target version** changed from _0.14.0_ to _0.15.0_

####

#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.

####

#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.

####

#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.

####

#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.

####

#18

Updated by Lucas Beeler 8 months ago

  * **Priority** changed from _Normal_ to _High_

####

#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 ?

####

#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.

####

#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.

####

#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.

####

#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:* &nbsp_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.

####

#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).

####

#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!

####

#26

Updated by Clinton Rogers 7 months ago

e03e0932d25edc31902ec836654c92d1f7626b95 is the exposure date/time caching
thing pushed into master and should improve speed a bit; more to come.

####

#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).

####

#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.

####

#29

Updated by Clinton Rogers 7 months ago

  * **Status** changed from _Open_ to _Review_

An updated patch is available; please see
9a5970a50d902b22306c31b64d3ef30c4168177e .

####

#30

Updated by Lucas Beeler 7 months ago

Review: Approve. Commit the revised version!

####

#31

Updated by Clinton Rogers 7 months ago

  * **Status** changed from _Review_ to _Open_
  * **% Done** changed from _40_ to _50_

Per Lucas, pushing cdeeef0a6eac22a4a32c3f3fc77ae17575b91d0a, but setting back
to open (still more to come).

####

#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...

####

#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](http://redmine.yorba.org/issues/979#note-32) below.

####

#34

Updated by Clinton Rogers 7 months ago

  * **Status** changed from _Open_ to _Review_

####

#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.

####

#36

Updated by Clinton Rogers 7 months ago

  * **Status** changed from _Open_ to _Review_

Please look to 5316448cd48cf1c969ee87061a0d04f209f600e3 for this.

####

#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.

####

#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."

####

#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.

####

#40

Updated by Joe Bylund 7 months ago

Just a question: is the same cost paid when creating a new event?

####

#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.

####

#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.

####

#43

Updated by Jim Nelson 6 months ago

  * **Target version** deleted (<strike>_0.15.0_</strike>)

####

#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