> -----Original Message----- > From: Kim Woelders [mailto:[email protected]] > Sent: 17. marts 2011 00:40 > To: [email protected]; Jesper Saxtorph > Subject: Re: [E-devel] imlib2 caching can fail > > On Sat, 12 Mar 2011 13:36:07 +0100, Jesper Saxtorph > <[email protected]> wrote: > > > On Friday 11 March 2011, 18:13:20, Jesper Saxtorph wrote: > >> > -----Original Message----- > >> > From: Kim Woelders [mailto:[email protected]] > >> > Sent: 10. marts 2011 20:52 > >> > To: Carsten Haitzler > >> > Cc: [email protected]; Jesper Saxtorph > >> > Subject: Re: [E-devel] imlib2 caching can fail > >> > > >> > On Thu, 10 Mar 2011 10:31:32 +0100, Carsten Haitzler > >> > > >> > <[email protected]> wrote: > >> > > On Sun, 06 Mar 2011 07:49:59 +0100 "Kim Woelders" <[email protected]> > >> > > >> > said: > >> > >> On Thu, 03 Mar 2011 15:46:15 +0100, Jesper Saxtorph > >> > >> > >> > >> <[email protected]> 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
I understand your thoughts, however I your suggestion will not work. I will try to explain it clearer. Bear with me, if it gets a little long and I repeat what is written earlier, but I am not sure what part you are missing. In an ideal world where timestamps has infinite precision and they represents the actual real time the files has been changed (never future timestamps). If this was the case the original code would work fine. In the real world 3 things can happen to break the setup. 1) People can force incorrect timestamps. 2) By error files can be marked in the future. If this happens we risk that the file will be changed again at the exact time of the timestamp, which means 2 writes with the same timestamp. 3) The file can be written more than one time with the same timestamp because of the timestamp resolution. Case 1), we should not really care about, as people break the system on purpose. For both case 2) and 3) the problem happen in the following situation: a) Imlib reads the file and register the timestamp of the file (for simplification I consider this atomic in this example). The file content is cached. b) The file is changed, but the timestamp is still the same. This can happen according to case 2) and 3) above. c) Imlib need the file data again and want (imlib_image_set_changes_on_disk) to decide if the cached data is valid. It checks the timestamp to validate this. However, as the timestamp has not changed, the data are deemed valid, while in reality they are not. Now back to your suggestion. What you do is the following: On first load you cache the data. On a later load you try test if the files current timestamp is right now. So lets take an example of why can fail according to case 3): I) A file is written at time 17. II) The file is read and cached by imlib at time 17. III) The file is changed again after the imlib read but still at time 17. IV) At time 20 imlib need the data again. It try to validate the cache. However, since the cache timestamp is still equal to the file timestamp and not equal to the current time (20), imlib will validate the cache, which is wrong. You can make a similar scenario for case 2) I hope this makes it more clear. -Jesper ------------------------------------------------------------------------------ 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 [email protected] https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
