unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
       [not found] ` <20220411113602.7A114C0581E@vcs2.savannah.gnu.org>
@ 2022-04-11 12:15   ` Po Lu
  2022-04-11 12:44     ` Lars Ingebrigtsen
  2022-04-11 13:12     ` Eli Zaretskii
  0 siblings, 2 replies; 16+ messages in thread
From: Po Lu @ 2022-04-11 12:15 UTC (permalink / raw)
  To: emacs-devel; +Cc: Lars Ingebrigtsen

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.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 12:15   ` master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too Po Lu
@ 2022-04-11 12:44     ` Lars Ingebrigtsen
  2022-04-11 12:56       ` Po Lu
  2022-04-11 13:12     ` Eli Zaretskii
  1 sibling, 1 reply; 16+ messages in thread
From: Lars Ingebrigtsen @ 2022-04-11 12:44 UTC (permalink / raw)
  To: Po Lu; +Cc: emacs-devel

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 12:44     ` Lars Ingebrigtsen
@ 2022-04-11 12:56       ` Po Lu
  2022-04-11 13:04         ` Lars Ingebrigtsen
  0 siblings, 1 reply; 16+ messages in thread
From: Po Lu @ 2022-04-11 12:56 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: emacs-devel

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.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 12:56       ` Po Lu
@ 2022-04-11 13:04         ` Lars Ingebrigtsen
  2022-04-11 13:12           ` Po Lu
  2022-04-11 13:58           ` Stefan Monnier
  0 siblings, 2 replies; 16+ messages in thread
From: Lars Ingebrigtsen @ 2022-04-11 13:04 UTC (permalink / raw)
  To: Po Lu; +Cc: emacs-devel

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 13:04         ` Lars Ingebrigtsen
@ 2022-04-11 13:12           ` Po Lu
  2022-04-11 13:15             ` Lars Ingebrigtsen
  2022-04-11 13:58           ` Stefan Monnier
  1 sibling, 1 reply; 16+ messages in thread
From: Po Lu @ 2022-04-11 13:12 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: emacs-devel

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 12:15   ` master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too Po Lu
  2022-04-11 12:44     ` Lars Ingebrigtsen
@ 2022-04-11 13:12     ` Eli Zaretskii
  2022-04-11 13:14       ` Lars Ingebrigtsen
                         ` (2 more replies)
  1 sibling, 3 replies; 16+ messages in thread
From: Eli Zaretskii @ 2022-04-11 13:12 UTC (permalink / raw)
  To: Po Lu; +Cc: larsi, emacs-devel

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 13:12     ` Eli Zaretskii
@ 2022-04-11 13:14       ` Lars Ingebrigtsen
  2022-04-11 13:22         ` Eli Zaretskii
  2022-04-11 13:25       ` Po Lu
  2022-04-11 14:55       ` Lars Ingebrigtsen
  2 siblings, 1 reply; 16+ messages in thread
From: Lars Ingebrigtsen @ 2022-04-11 13:14 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Po Lu, emacs-devel

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 13:12           ` Po Lu
@ 2022-04-11 13:15             ` Lars Ingebrigtsen
  2022-04-11 13:29               ` Po Lu
  0 siblings, 1 reply; 16+ messages in thread
From: Lars Ingebrigtsen @ 2022-04-11 13:15 UTC (permalink / raw)
  To: Po Lu; +Cc: emacs-devel

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 13:14       ` Lars Ingebrigtsen
@ 2022-04-11 13:22         ` Eli Zaretskii
  0 siblings, 0 replies; 16+ messages in thread
From: Eli Zaretskii @ 2022-04-11 13:22 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: luangruo, emacs-devel

> 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);
    }



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 13:12     ` Eli Zaretskii
  2022-04-11 13:14       ` Lars Ingebrigtsen
@ 2022-04-11 13:25       ` Po Lu
  2022-04-11 13:29         ` Eli Zaretskii
  2022-04-11 14:55       ` Lars Ingebrigtsen
  2 siblings, 1 reply; 16+ messages in thread
From: Po Lu @ 2022-04-11 13:25 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: emacs-devel, larsi

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?



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 13:15             ` Lars Ingebrigtsen
@ 2022-04-11 13:29               ` Po Lu
  0 siblings, 0 replies; 16+ messages in thread
From: Po Lu @ 2022-04-11 13:29 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: emacs-devel

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!



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 13:25       ` Po Lu
@ 2022-04-11 13:29         ` Eli Zaretskii
  2022-04-11 13:32           ` Po Lu
  0 siblings, 1 reply; 16+ messages in thread
From: Eli Zaretskii @ 2022-04-11 13:29 UTC (permalink / raw)
  To: Po Lu; +Cc: larsi, emacs-devel

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 13:29         ` Eli Zaretskii
@ 2022-04-11 13:32           ` Po Lu
  0 siblings, 0 replies; 16+ messages in thread
From: Po Lu @ 2022-04-11 13:32 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, emacs-devel

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.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 13:04         ` Lars Ingebrigtsen
  2022-04-11 13:12           ` Po Lu
@ 2022-04-11 13:58           ` Stefan Monnier
  2022-04-11 14:55             ` Lars Ingebrigtsen
  1 sibling, 1 reply; 16+ messages in thread
From: Stefan Monnier @ 2022-04-11 13:58 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Po Lu, emacs-devel

> 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




^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 13:12     ` Eli Zaretskii
  2022-04-11 13:14       ` Lars Ingebrigtsen
  2022-04-11 13:25       ` Po Lu
@ 2022-04-11 14:55       ` Lars Ingebrigtsen
  2 siblings, 0 replies; 16+ messages in thread
From: Lars Ingebrigtsen @ 2022-04-11 14:55 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Po Lu, emacs-devel

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too
  2022-04-11 13:58           ` Stefan Monnier
@ 2022-04-11 14:55             ` Lars Ingebrigtsen
  0 siblings, 0 replies; 16+ messages in thread
From: Lars Ingebrigtsen @ 2022-04-11 14:55 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: Po Lu, emacs-devel

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



^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2022-04-11 14:55 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <164967696186.12152.11548736665906939483@vcs2.savannah.gnu.org>
     [not found] ` <20220411113602.7A114C0581E@vcs2.savannah.gnu.org>
2022-04-11 12:15   ` master 5141234acf: Refactor the webp cache code to allow usage by gif_load, too Po Lu
2022-04-11 12:44     ` Lars Ingebrigtsen
2022-04-11 12:56       ` Po Lu
2022-04-11 13:04         ` Lars Ingebrigtsen
2022-04-11 13:12           ` Po Lu
2022-04-11 13:15             ` Lars Ingebrigtsen
2022-04-11 13:29               ` Po Lu
2022-04-11 13:58           ` Stefan Monnier
2022-04-11 14:55             ` Lars Ingebrigtsen
2022-04-11 13:12     ` Eli Zaretskii
2022-04-11 13:14       ` Lars Ingebrigtsen
2022-04-11 13:22         ` Eli Zaretskii
2022-04-11 13:25       ` Po Lu
2022-04-11 13:29         ` Eli Zaretskii
2022-04-11 13:32           ` Po Lu
2022-04-11 14:55       ` Lars Ingebrigtsen

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).