Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> +  void (*destructor)(void*);

Shouldn't this be formatted like:

  void (*destructor) (void *);

instead?

> +  cache->spec = spec;

I think this should be marked, since I don't see what protects the spec
from garbage collection otherwise.

Thanks.

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> Shouldn't this be formatted like:
>
>   void (*destructor) (void *);
>
> instead?

Yup.

>> +  cache->spec = spec;
>
> I think this should be marked, since I don't see what protects the spec
> from garbage collection otherwise.

I suspect so, but as the comment on the previous line says, I wasn't
quite sure.  We're only using it to test for EQ-ness...  but I guess
that will break if we're using the complex Lisp object type.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I suspect so, but as the comment on the previous line says, I wasn't
> quite sure.  We're only using it to test for EQ-ness...  but I guess
> that will break if we're using the complex Lisp object type.

The object might be garbage collected, and another one could be consed
with the same type tag and address as the original.

So we should probably mark that data upon garbage collection.

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> The object might be garbage collected, and another one could be consed
> with the same type tag and address as the original.

It seems rather unlikely -- especially since it's also checking that the
index is the expected one.

> So we should probably mark that data upon garbage collection.

For the imagemagick cache, we're just using a cookie, that's basically
what we're doing here, too.  Perhaps sxhashing the spec and using that
instead would be fine, but it's slower, and we don't really care that
much about correctness here -- we're just using this to speed things up,
after all.

So I dunno.  My only worry was that not gc-protecting it might lead to a
segfault somewhere.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> For the imagemagick cache, we're just using a cookie, that's basically
> what we're doing here, too.  Perhaps sxhashing the spec and using that
> instead would be fine, but it's slower, and we don't really care that
> much about correctness here -- we're just using this to speed things up,
> after all.

Why not put something like this inside mark_image_cache?

#if defined HAVE_WEBP || defined HAVE_GIF
for (struct anim_cache *cache = anim_cache;
     cache; cache = cache->next)
  mark_object (cache->spec);
#endif

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> Why not put something like this inside mark_image_cache?
>
> #if defined HAVE_WEBP || defined HAVE_GIF
> for (struct anim_cache *cache = anim_cache;
>      cache; cache = cache->next)
>   mark_object (cache->spec);
> #endif

Oh, yeah, that's a good idea.  Thanks -- I thought I'd have to do
something more complicated.

I'll push to Emacs 29 in a moment.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Po Lu]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
>> Why not put something like this inside mark_image_cache?
>>
>> #if defined HAVE_WEBP || defined HAVE_GIF
>> for (struct anim_cache *cache = anim_cache;
>>      cache; cache = cache->next)
>>   mark_object (cache->spec);
>> #endif
>
> Oh, yeah, that's a good idea.  Thanks -- I thought I'd have to do
> something more complicated.

Any time, thanks!

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Stefan Monnier]

> So I dunno.  My only worry was that not gc-protecting it might lead to a
> segfault somewhere.

I think a call to `mark_object` is much cheaper than this discussion.


        Stefan

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> I think a call to `mark_object` is much cheaper than this discussion.

Very true.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 11 Apr 2022 20:15:12 +0800
> 
> > +  cache->spec = spec;
> 
> I think this should be marked, since I don't see what protects the spec
> from garbage collection otherwise.

'spec' here is img->spec, right?

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> 'spec' here is img->spec, right?

Yup.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Po Lu <luangruo@yahoo.com>,  emacs-devel@gnu.org
> Date: Mon, 11 Apr 2022 15:14:20 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > 'spec' here is img->spec, right?
> 
> Yup.

So it's already marked when GC runs:

    static void
    mark_image (struct image *img)
    {
      mark_object (img->spec); <<<<<<<<<<<<<<<<<<<<<<<<<<<<<
      mark_object (img->dependencies);

      if (!NILP (img->lisp_data))
	mark_object (img->lisp_data);
    }

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: Lars Ingebrigtsen <larsi@gnus.org>
>> Date: Mon, 11 Apr 2022 20:15:12 +0800
>> 
>> > +  cache->spec = spec;
>> 
>> I think this should be marked, since I don't see what protects the spec
>> from garbage collection otherwise.
>
> 'spec' here is img->spec, right?

Right, but the animation cache can persist after img is gone, right?

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  larsi@gnus.org
> Date: Mon, 11 Apr 2022 21:25:07 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Po Lu <luangruo@yahoo.com>
> >> Cc: Lars Ingebrigtsen <larsi@gnus.org>
> >> Date: Mon, 11 Apr 2022 20:15:12 +0800
> >> 
> >> > +  cache->spec = spec;
> >> 
> >> I think this should be marked, since I don't see what protects the spec
> >> from garbage collection otherwise.
> >
> > 'spec' here is img->spec, right?
> 
> Right, but the animation cache can persist after img is gone, right?

It can?  Then how does this work:

      struct anim_cache* cache = anim_get_animation_cache (img->spec);

We pass img->spec to look it up in the cache, don't we?  And we use EQ
to look it up, right?

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> It can?  Then how does this work:
>
>       struct anim_cache* cache = anim_get_animation_cache (img->spec);
>
> We pass img->spec to look it up in the cache, don't we?  And we use EQ
> to look it up, right?

Nevermind, thanks.  I got confused in my reading of that code.

Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> I think this should be marked, since I don't see what protects the spec
>> from garbage collection otherwise.
>
> 'spec' here is img->spec, right?

But the spec in the cache might be from a different img (we're looking
through the cache for the right one), so the spec there may have been
gc'd.  So Po Lu's fix was the right one here, I think (and has been
pushed now).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no