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