On Sat, 12 Mar 2011 13:36:07 +0100, Jesper Saxtorph <jesper.saxto...@prevas.dk> wrote:
> On Friday 11 March 2011, 18:13:20, Jesper Saxtorph wrote: >> > -----Original Message----- >> > From: Kim Woelders [mailto:k...@woelders.dk] >> > Sent: 10. marts 2011 20:52 >> > To: Carsten Haitzler >> > Cc: enlightenment-devel@lists.sourceforge.net; Jesper Saxtorph >> > Subject: Re: [E-devel] imlib2 caching can fail >> > >> > On Thu, 10 Mar 2011 10:31:32 +0100, Carsten Haitzler >> > >> > <ras...@rasterman.com> wrote: >> > > On Sun, 06 Mar 2011 07:49:59 +0100 "Kim Woelders" <k...@woelders.dk> >> > >> > said: >> > >> On Thu, 03 Mar 2011 15:46:15 +0100, Jesper Saxtorph >> > >> >> > >> <jesper.saxto...@prevas.dk> wrote: >> > >> > I have just submitted a bugraport + a patch on trac (bug #716). >> > >> > >> > >> > I just wanted to notifify it here also as I do not know the >> > >> > enlightenment community and how it works. >> > >> >> > >> Either is fine. >> > >> >> > >> > To repeat my bug report: >> > >> > "I use imlib2 as a image library in a project. However the use we >> > >> > have >> > >> > >> > sometimes tricker a situation where imlib2 uses an invalid cache. >> > >> > >> > >> > imlib2 uses timestamps to test if a image cache i valid. If a >> >> files >> >> > >> > modification time is in the future it is not possible to use >> > >> >> > >> validation >> > >> >> > >> > scheme. Further if the timestamp is equal to now, we do not know >> >> if >> >> > >> the >> > >> >> > >> > modification time is in the future or not. The result is that the >> > >> >> > >> cache >> > >> >> > >> > should be invalidated for file timestamps >= now. >> > >> > An example of a problematic situation: timestamps are in whole >> > >> > seconds >> > >> > >> > times given as here as seconds: >> > >> > time=32.1 : image.png is written by someone >> > >> > time=32.4 : image.png is loaded by imlib2 >> > >> > time=32.5 : image.png is written with new data >> > >> > The situation is now that the cache has the same timestamp as the >> > >> >> > >> file, >> > >> >> > >> > but the content is not the same. >> > >> > >> > >> > A possible fix is a 3 line patch, which I have attached. It >> > >> > invalidate >> > >> > >> > the cache if the files timestamp is >= now. >> > >> > The patch is made agains head, however, the image.c file (where >> >> the >> >> > >> > patch is applied) has changed very little in it lifetime, so it >> >> works >> >> > >> > fine with earlier versions of imlib2 (I use it against 1.4.2)." >> > >> > >> > >> > I attached an incorrect patch at first to the ticket, but have >> > >> >> > >> submittet >> > >> >> > >> > the correct afterwards. Sorry for that. >> > >> > >> > >> > My suggestion to a patch (I have made the long comment as I think >> >> it >> >> > >> is >> > >> >> > >> > not obvious why it is needed): >> > >> > --- imlib2/src/lib/image.c.orig 2011-03-03 14:23:49.000000000 >> >> +0100 >> >> > >> > +++ imlib2/src/lib/image.c 2011-03-03 14:45:19.000000000 >> >> +0100 >> >> > >> > @@ -1017,6 +1017,18 @@ >> > >> > >> > >> > im->key = __imlib_FileKey(file); >> > >> > >> > >> > } >> > >> > >> > >> > im->moddate = __imlib_FileModDate(file); >> > >> > >> > >> > + /* If the file modify time is now or in the future, we can >> >> not >> >> > >> make >> > >> >> > >> > a */ >> > >> > + /* cache. */ >> > >> > + /* One of several possible scenarios: */ >> > >> > + /* time=now: file is written by someone */ >> > >> > + /* time=now: file is loaded here */ >> > >> > + /* time=now: file is written again by someone */ >> > >> > + /* Now we have a file a timestamp equal to our cache, but >> >> with a >> >> > >> */ >> > >> >> > >> > + /* different content. */ >> > >> > + if (im->moddate >= time(NULL)) >> > >> > + { >> > >> > + dont_cache = 1; >> > >> > + } >> > >> > >> > >> > /* ok - just check all our loaders are up to date */ >> > >> > __imlib_RescanLoaders(); >> > >> > /* take a guess by extension on the best loader to use */ >> > >> > >> > >> > Hope it make sense, otherwise feel free to ask and/or discuss it >> > >> >> > >> I see there is a problem, but I don't think it is the proper >> >> solution. >> >> > >> If you have a file with a "now"/future time stamp it would never be >> > >> cached, which is wrong. >> > >> >> > >> How about in stead changing line 986 (svn) to check whether the >> >> time >> >> > >> stamp >> > >> has changed in stead of checking if it is newer? >> > > >> > > while in general you are also right.. he's also trying to fix the >> > > problem of >> > > "write file at 34.1 secs, then write AGAIN at 34.5 secs" - they both >> > > have the >> > > SAME timestamp but the 2nd is newer. imlib2 will not load the newer >> >> file >> >> > > as the >> > > timestamps match due to timestamp resolution (34 vs 34 secs). but >> >> while >> >> > > solving >> > > this problem it breaks caching for timestamp == now. you'd need >> >> much >> >> > > higher >> > > resolution timestamps (and even then you still just have a smaller >> >> race >> >> > > condition) or .. you'd have to have another way to differentiate the >> > > files. >> > > file size + timestamp + inode + .... even then you just lower the >> > > probability >> > > not eliminate it. >> > >> > Yeah, I know. >> > >> > The important thing here, IMO, is that images which never change, >> >> should >> >> > always be cached (unless explicitly not caching), whichever >> > past/now/future time stamp they may have. >> > >> > How about this: >> > >> > diff --git src/lib/image.c src/lib/image.c >> > index d404961..e8610b2 100644 >> > --- src/lib/image.c >> > +++ src/lib/image.c >> > @@ -982,8 +982,10 @@ __imlib_LoadImage(const char *file, >> > ImlibProgressFunction progres >> > >> > time_t current_modified_time; >> > >> > current_modified_time = __imlib_FileModDate(file); >> > >> > - /* if the file on disk is newer than the cached one */ >> > - if (current_modified_time > im->moddate) >> > + /* if the disk file time stamp is different from the */ >> > + /* cached one or appears to have just been set */ >> > + if ((current_modified_time != im->moddate) || >> > + (current_modified_time == time(NULL))) >> > >> > { >> > >> > /* invalidate image */ >> > SET_FLAG(im->flags, F_INVALID); >> > >> > This of course requires using imlib_image_set_changes_on_disk() in the >> > application. >> >> If I remember right, setting the F_INVALID flag will invalidate the >> current (old) cach and not prevent caching what we are about to load. >> >> The problem I describe is that when we load an image with a timestamp >> now or in the future, we will never be able to see if there is a never >> image on the disk afterwards. Therefore you can not check and invalidate >> the cach later on, as you try to do. >> If you take my sequence of write image -> imlib load it -> write it >> again within the same timestamp, you will not be able to detect that it >> has happened afterwards. Your suggestion will simple invalidate any old >> cach sitting around, then load the image and cach it and never see it >> has changed again within the same timestamp. >> >> However, as I have been looking at the code again, I found that you are >> right that dont_cache will make it never cache the image, as it test >> dont_cache in the end of the function and set F_UNCACHEABLE. >> I still think the idea of my code is the best way to go, however it need >> to be adjusted so it is only the current caching which is avoided. >> >> I wondered why the code was made like that and began to look further. I >> found that the comments in the code do not match the behaviour. >> imlib_load_image_without_cache and >> imlib_load_image_immediately_without_cache in api.c has comments stating >> they will load the image "without looking in the cache first", but do >> not state it will not be cached for further use. >> As far as I can see, these functions WILL use the cache, if the cache is >> valid. However, they will not cache what they load. Also they will set >> the F_UNCACHEABLE flag. >> As I have not had time to look further into it, I am not sure if the >> intention here is what the comments states or what the code do. > > How about this one: > > Index: src/lib/image.c > =================================================================== > --- src/lib/image.c (revision 57702) > +++ src/lib/image.c (working copy) > @@ -1017,6 +1017,11 @@ > im->key = __imlib_FileKey(file); > } > im->moddate = __imlib_FileModDate(file); > + /* If we can not validate the timestamp, make sure it is always old. > */ > + if (im->moddate >= time(NULL)) > + { > + im->moddate = 0; > + } > /* ok - just check all our loaders are up to date */ > __imlib_RescanLoaders(); > /* take a guess by extension on the best loader to use */ > > This will only influence the situation when checking cache by using > imlib_image_set_changes_on_disk() and further will only influence the > next > check, which will lead to a reload. > This still makes files with a future time stamp uncached when using imlib_image_set_changes_on_disk(). What was wrong with my suggestion? /Kim ------------------------------------------------------------------------------ Colocation vs. Managed Hosting A question and answer guide to determining the best fit for your organization - today and in the future. http://p.sf.net/sfu/internap-sfd2d _______________________________________________ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
