From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Kangas Newsgroups: gmane.emacs.bugs Subject: bug#45224: 28.0.50; eww and GIFS (cpu usage shoots through the roof) Date: Sat, 30 Oct 2021 18:54:56 -0700 Message-ID: References: <87r1ntnabo.fsf@gmail.com> <87eejsz5aw.fsf@gnus.org> <87lfe0fbuv.fsf@gmail.com> <877dpkfbf9.fsf@gmail.com> <874kkn39t3.fsf@gnus.org> <87h7ojlage.fsf@gmail.com> <87h7ojjvhd.fsf@gnus.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="000000000000e6285305cf9c58bc" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="25734"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/29.0.50 (gnu/linux) Cc: Madhavan Krishnan , 45224@debbugs.gnu.org To: Lars Ingebrigtsen Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Sun Oct 31 02:56:10 2021 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mh04w-0006Xp-Lc for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 31 Oct 2021 02:56:10 +0100 Original-Received: from localhost ([::1]:56952 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mh04u-0001hM-Js for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 30 Oct 2021 21:56:08 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:47334) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mh04o-0001gy-6y for bug-gnu-emacs@gnu.org; Sat, 30 Oct 2021 21:56:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:47161) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mh04n-0007KA-Uw for bug-gnu-emacs@gnu.org; Sat, 30 Oct 2021 21:56:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mh04n-00019d-J5 for bug-gnu-emacs@gnu.org; Sat, 30 Oct 2021 21:56:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Kangas Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 31 Oct 2021 01:56:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 45224 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: confirmed Original-Received: via spool by 45224-submit@debbugs.gnu.org id=B45224.16356453054361 (code B ref 45224); Sun, 31 Oct 2021 01:56:01 +0000 Original-Received: (at 45224) by debbugs.gnu.org; 31 Oct 2021 01:55:05 +0000 Original-Received: from localhost ([127.0.0.1]:58707 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mh03t-00018F-24 for submit@debbugs.gnu.org; Sat, 30 Oct 2021 21:55:05 -0400 Original-Received: from mail-pf1-f178.google.com ([209.85.210.178]:35828) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mh03q-00017h-Rt for 45224@debbugs.gnu.org; Sat, 30 Oct 2021 21:55:03 -0400 Original-Received: by mail-pf1-f178.google.com with SMTP id s5so1429907pfg.2 for <45224@debbugs.gnu.org>; Sat, 30 Oct 2021 18:55:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:in-reply-to:references:user-agent :mime-version:date:message-id:subject:to:cc; bh=eJGqlZTS1GrU+jeHEZGukZxttZfPrSeiqPlDWqIU1xo=; b=Qm8b3lKHjb2oN4Saz+WzAiqY8rbFJTjMUyT6yEkuVKioPpBmgRk0fgzNqCnnc3aiI1 bNN0NbCSme0EfRcbsQVqCmG3sOw+3S87TiPVatrSXqNcbog0j+N/ViOETPuMGgsDECEK 2IgH4cz3E8ET3qEY80Uv5SdPxQYLOO0l0bslOQJvcSGLE1dgi+lTuQP6v5aCrlZMgH0g aoBlrrIC3kuXPDz3qA9HBSD2kX5n9chDShFRqwW9qk6BlZhsDECD7UeGvvSXkyxrN4G+ zfhlltSEtVb66kRz6FkLd1m4+jhY1WfVpgKDlm9EUEDBvAQTONuBYTfd9TLvS0GtwfMu ANbQ== X-Gm-Message-State: AOAM530ejb4iBmb7lFnL2FYWTgLm3IZZAUSYDvYp5XqW14Qs9m+gxCi8 jKTHT69Kmgd21wo+wO/N9hVNKp2PapBYa7qYcS8= X-Google-Smtp-Source: ABdhPJxl1d4AL818ulQXVKPzD360RdDcyvChgsU2Hm00GNWTfLxKHAnCDcWtccMoMIJ0+PKAEQxjHuF4RdXMxeY1vzM= X-Received: by 2002:a63:86c1:: with SMTP id x184mr6033933pgd.114.1635645296871; Sat, 30 Oct 2021 18:54:56 -0700 (PDT) Original-Received: from 753933720722 named unknown by gmailapi.google.com with HTTPREST; Sat, 30 Oct 2021 18:54:56 -0700 In-Reply-To: <87h7ojjvhd.fsf@gnus.org> (Lars Ingebrigtsen's message of "Fri, 18 Dec 2020 10:48:14 +0100") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:218648 Archived-At: --000000000000e6285305cf9c58bc Content-Type: text/plain; charset="UTF-8" Lars Ingebrigtsen writes: > Madhavan Krishnan writes: > >> Thank you for investigating this issue, I am not entirely sure of what >> the next step would be. Is there a sample code that I can refer to for >> the ImageMagic version you are referring to? (to get some idea about how >> it can be patched) > > imagemagick_get_animation_cache and friends in src/image.c. I've been looking into this a bit, and here are my preliminary conclusions. I hope it is clear, and appreciate any feedback! The first part of the problem is making sure that we can cache animated images, which currently never happens. Once that is in place, the second part is to cache all animated images up-front. I only discuss the first part below. Besides the special cache for ImageMagick we also have tho one used for *all* images. So my thinking is: why not use the "real" cache also for animated images? Why implement a specialized cache just for that? I believe it is possible.[1] And actually, the first question is: why aren't they already cached? The reason is this: in search_image_cache (image.c:1599), we use sxhash and Fequal to compare the Lisp image descriptor (or "image specification" or "spec" as its called in image.c) to the one we have in cache. But in an animated gif the image spec is changed in image.el: (image :type gif :file "/some/file.gif" :scale 1 :max-width 1248 :max-height 1321 :format nil :transform-smoothing t :animate-buffer "file.gif" :animate-tardiness 0.9810895737588246 :animate-multi-frame-data (62 . 0.04) :index 61) The value of :animate-tardiness is updated in image.el on every iteration. When image.el updates :animate-tardiness, it will never be equal to something in the cache and we always get a cache miss! I have attached a patch that will demonstrate how we get back caching by simply disabling the updating of the image spec in image.el. This is completely the wrong thing to do, but demonstrates what is going on. With that patch, caching will be enabled, and you can see it the second time you try to run an animated gif. (Use RET to start the animation.) The correct fix here is to somehow *only* compare the relevant attributes above, and ignore or leave out e.g. :animate-tardiness. I have experimented with different approaches. My first idea was to create a new comparison function "image_cache_equal" that basically only compares the attributes we are interested. But then the problem was that we use the "sxhash" function to find which hash bucket to find the list in, so that didn't help much. We would also need to replace the sxhash with something else. Perhaps sxhash_equal? Hmm. So I thought of two other ways to go about this: A) We create a new stripped down Lisp plist containing only the attributes we are interested in and use that to compare against the cache. We obviously need to make sure the cache also contains only stripped down list. This means we can keep using Fequal and sxhash, but we also need to create a new list for *every cache lookup*! B) We parse the relevant parts of the image spec into a C struct and then use that struct for comparisons. It turns out we already have most of the code in place to do this, see parse_image_spec (image.c:898), so most of it should be some restructuring, and then writing up a comparison function. With that in place, we would just need to . Option A) seems ugly to me: why would we be consing up Lisp lists on such a low level, which also makes me worry about creating a lot of unnecessary garbage. So I prefer Option B). The struct also seems more clean to me. Perhaps there are some performance implications I'm not thinking of? Or perhaps there is some even better way to do the cache check than a new struct, such as using the "image struct" directly? A third alternative is to somehow change image.el to put this information outside the image specifier, but that leaves unfixed a rather subtle issue with caching. That issue may or may not bite someone later. Comments about all this is very welcome! I'm basically at the point where any approach I choose means investing a bunch of work, and it would be useful with some feedback before I rush ahead and attempt any of them. Perhaps someone here has an idea or hunch about which approach might prove more fruitful. Footnotes: [1] There is a comment in the ImageMagick cache saying: "We have to maintain a cache separate from the image cache, because the images may be scaled before display." However, this was written before we had native image scaling, and actually it seems to me either incorrect, based on the above, or correct but only applicable to ImageMagick, or, perhaps more likely, I am missing something important. --000000000000e6285305cf9c58bc Content-Type: text/x-diff; charset="US-ASCII"; name="0001-gif-load-debug.patch" Content-Disposition: attachment; filename="0001-gif-load-debug.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: 22cc92d41ad133c9_0.1 RnJvbSAyMDQxMDgzYjYxMWMwMDI4ODJlYTVhZDJlMzM0MWJmNjNiOGM5NGI0IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBTdGVmYW4gS2FuZ2FzIDxzdGVmYW5AbWFyeGlzdC5zZT4KRGF0 ZTogU3VuLCAzMSBPY3QgMjAyMSAwMTozNzozMSArMDIwMApTdWJqZWN0OiBbUEFUQ0hdIGdpZiBs b2FkIGRlYnVnCgotLS0KIGxpc3AvaW1hZ2UuZWwgfCAzNyArKysrKysrKysrKysrKysrKysrKy0t LS0tLS0tLS0tLS0tLS0tCiBzcmMvaW1hZ2UuYyAgIHwgIDMgKysrCiAyIGZpbGVzIGNoYW5nZWQs IDIzIGluc2VydGlvbnMoKyksIDE3IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL2xpc3AvaW1h Z2UuZWwgYi9saXNwL2ltYWdlLmVsCmluZGV4IGJmY2JiNjZjMjUuLjNhYjQxNGE0ZDMgMTAwNjQ0 Ci0tLSBhL2xpc3AvaW1hZ2UuZWwKKysrIGIvbGlzcC9pbWFnZS5lbApAQCAtODQxLDEwICs4NDEs MTAgQEAgaW1hZ2UtYW5pbWF0ZQogICAgICAgKGlmIChzZXRxIHRpbWVyIChpbWFnZS1hbmltYXRl LXRpbWVyIGltYWdlKSkKIAkgIChjYW5jZWwtdGltZXIgdGltZXIpKQogICAgICAgKHBsaXN0LXB1 dCAoY2RyIGltYWdlKSA6YW5pbWF0ZS1idWZmZXIgKGN1cnJlbnQtYnVmZmVyKSkKLSAgICAgIChw bGlzdC1wdXQgKGNkciBpbWFnZSkgOmFuaW1hdGUtdGFyZGluZXNzIDApCisgICAgICA7OyAocGxp c3QtcHV0IChjZHIgaW1hZ2UpIDphbmltYXRlLXRhcmRpbmVzcyAwKQogICAgICAgOzsgU3Rhc2gg dGhlIGRhdGEgYWJvdXQgdGhlIGFuaW1hdGlvbiBoZXJlIHNvIHRoYXQgd2UgZG9uJ3QKICAgICAg IDs7IHRyaWdnZXIgaW1hZ2UgcmVjb21wdXRhdGlvbiB1bm5lY2Vzc2FyaWx5IGxhdGVyLgotICAg ICAgKHBsaXN0LXB1dCAoY2RyIGltYWdlKSA6YW5pbWF0ZS1tdWx0aS1mcmFtZS1kYXRhIGFuaW1h dGlvbikKKyAgICAgIDs7IChwbGlzdC1wdXQgKGNkciBpbWFnZSkgOmFuaW1hdGUtbXVsdGktZnJh bWUtZGF0YSBhbmltYXRpb24pCiAgICAgICAocnVuLXdpdGgtdGltZXIgMC4yIG5pbCAjJ2ltYWdl LWFuaW1hdGUtdGltZW91dAogCQkgICAgICBpbWFnZSAob3IgaW5kZXggMCkgKGNhciBhbmltYXRp b24pCiAJCSAgICAgIDAgbGltaXQgKCsgKGZsb2F0LXRpbWUpIDAuMikpKSkpCkBAIC04NzMsMTAg Kzg3MywxMCBAQCBpbWFnZS1zaG93LWZyYW1lCiAgICJTaG93IGZyYW1lIE4gb2YgSU1BR0UuCiBG cmFtZXMgYXJlIGluZGV4ZWQgZnJvbSAwLiAgT3B0aW9uYWwgYXJndW1lbnQgTk9DSEVDSyBub24t bmlsIG1lYW5zCiBkbyBub3QgY2hlY2sgTiBpcyB3aXRoaW4gdGhlIHJhbmdlIG9mIGZyYW1lcyBw cmVzZW50IGluIHRoZSBpbWFnZS4iCi0gICh1bmxlc3Mgbm9jaGVjawotICAgIChpZiAoPCBuIDAp IChzZXRxIG4gMCkKLSAgICAgIChzZXRxIG4gKG1pbiBuICgxLSAoY2FyIChwbGlzdC1nZXQgKGNk ciBpbWFnZSkKLSAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgOmFuaW1h dGUtbXVsdGktZnJhbWUtZGF0YSkpKSkpKSkKKyAgOzsgKHVubGVzcyBub2NoZWNrCisgIDs7ICAg KGlmICg8IG4gMCkgKHNldHEgbiAwKQorICA7OyAgICAgKHNldHEgbiAobWluIG4gKDEtIChjYXIg KHBsaXN0LWdldCAoY2RyIGltYWdlKQorICA7OyAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICA6YW5pbWF0ZS1tdWx0aS1mcmFtZS1kYXRhKSkpKSkpKQogICAocGxpc3QtcHV0 IChjZHIgaW1hZ2UpIDppbmRleCBuKQogICAoZm9yY2Utd2luZG93LXVwZGF0ZSAocGxpc3QtZ2V0 IChjZHIgaW1hZ2UpIDphbmltYXRlLWJ1ZmZlcikpKQogCkBAIC05MTIsMjMgKzkxMiwyNiBAQCBp bWFnZS1hbmltYXRlLXRpbWVvdXQKIGZvciB0aGUgYW5pbWF0aW9uIHNwZWVkLiAgQSBuZWdhdGl2 ZSB2YWx1ZSBtZWFucyB0byBhbmltYXRlIGluIHJldmVyc2UuIgogICA7OyBXZSBrZWVwIHRyYWNr IG9mICJob3cgbGF0ZSIgaW1hZ2UgZnJhbWVzIGFycml2ZS4gIFdlIGRlY2F5IHRoZQogICA7OyBw cmV2aW91cyBjdW11bGF0aXZlIHZhbHVlIGJ5IDEwJSBhbmQgdGhlbiBhZGQgdGhlIGN1cnJlbnQg ZGVsYXkuCi0gIChwbGlzdC1wdXQgKGNkciBpbWFnZSkgOmFuaW1hdGUtdGFyZGluZXNzCi0gICAg ICAgICAgICAgKCsgKCogKHBsaXN0LWdldCAoY2RyIGltYWdlKSA6YW5pbWF0ZS10YXJkaW5lc3Mp IDAuOSkKLSAgICAgICAgICAgICAgICAoZmxvYXQtdGltZSAodGltZS1zaW5jZSB0YXJnZXQtdGlt ZSkpKSkKKyAgOzsgKHBsaXN0LXB1dCAoY2RyIGltYWdlKSA6YW5pbWF0ZS10YXJkaW5lc3MKKyAg OzsgICAgICAgICAgICAoKyAoKiAocGxpc3QtZ2V0IChjZHIgaW1hZ2UpIDphbmltYXRlLXRhcmRp bmVzcykgMC45KQorICA7OyAgICAgICAgICAgICAgIChmbG9hdC10aW1lICh0aW1lLXNpbmNlIHRh cmdldC10aW1lKSkpKQogICAod2hlbiAoYW5kIChidWZmZXItbGl2ZS1wIChwbGlzdC1nZXQgKGNk ciBpbWFnZSkgOmFuaW1hdGUtYnVmZmVyKSkKICAgICAgICAgICAgICA7OyBDdW11bGF0aXZlbHkg ZGVsYXllZCB0d28gc2Vjb25kcyBtb3JlIHRoYW4gZXhwZWN0ZWQuCi0gICAgICAgICAgICAgKG9y ICg8IChwbGlzdC1nZXQgKGNkciBpbWFnZSkgOmFuaW1hdGUtdGFyZGluZXNzKSAyKQotCQkgKHBy b2duCi0JCSAgIChtZXNzYWdlICJTdG9wcGluZyBhbmltYXRpb247IGFuaW1hdGlvbiBwb3NzaWJs eSB0b28gYmlnIikKLQkJICAgbmlsKSkpCisgICAgICAgICAgICAgdAorICAgICAgICAgICAgIDs7 IChvciAoPCAocGxpc3QtZ2V0IChjZHIgaW1hZ2UpIDphbmltYXRlLXRhcmRpbmVzcykgMikKKyAg ICAgICAgICAgICA7OyAgICAgKHByb2duCisgICAgICAgICAgICAgOzsgICAgICAgKG1lc3NhZ2Ug IlN0b3BwaW5nIGFuaW1hdGlvbjsgYW5pbWF0aW9uIHBvc3NpYmx5IHRvbyBiaWciKQorICAgICAg ICAgICAgIDs7ICAgICAgIG5pbCkpCisgICAgICAgICAgICAgKQogICAgIChpbWFnZS1zaG93LWZy YW1lIGltYWdlIG4gdCkKICAgICAobGV0KiAoKHNwZWVkIChpbWFnZS1hbmltYXRlLWdldC1zcGVl ZCBpbWFnZSkpCiAJICAgKHRpbWUgKGN1cnJlbnQtdGltZSkpCiAJICAgKHRpbWUtdG8tbG9hZC1p bWFnZSAodGltZS1zaW5jZSB0aW1lKSkKLQkgICAoc3RhdGVkLWRlbGF5LXRpbWUKLSAgICAgICAg ICAgICgvIChvciAoY2RyIChwbGlzdC1nZXQgKGNkciBpbWFnZSkgOmFuaW1hdGUtbXVsdGktZnJh bWUtZGF0YSkpCi0JCSAgIGltYWdlLWRlZmF1bHQtZnJhbWUtZGVsYXkpCi0JICAgICAgIChmbG9h dCAoYWJzIHNwZWVkKSkpKQorICAgICAgICAgICAoc3RhdGVkLWRlbGF5LXRpbWUgMC4wNworICAg ICAgICAgICAgOzsgKC8gKG9yIChjZHIgKHBsaXN0LWdldCAoY2RyIGltYWdlKSA6YW5pbWF0ZS1t dWx0aS1mcmFtZS1kYXRhKSkKKyAgICAgICAgICAgIDs7ICAgICAgICBpbWFnZS1kZWZhdWx0LWZy YW1lLWRlbGF5KQorICAgICAgICAgICAgOzsgICAgKGZsb2F0IChhYnMgc3BlZWQpKSkKKyAgICAg ICAgICAgICkKIAkgICA7OyBTdWJ0cmFjdCBvZmYgdGhlIHRpbWUgd2UgdG9vayB0byBsb2FkIHRo ZSBpbWFnZSBmcm9tIHRoZQogCSAgIDs7IHN0YXRlZCBkZWxheSB0aW1lLgogCSAgIChkZWxheSAo bWF4IChmbG9hdC10aW1lICh0aW1lLXN1YnRyYWN0IHN0YXRlZC1kZWxheS10aW1lCmRpZmYgLS1n aXQgYS9zcmMvaW1hZ2UuYyBiL3NyYy9pbWFnZS5jCmluZGV4IDJlYzQ0NTg0YjYuLmI4N2Q1MGQx N2YgMTAwNjQ0Ci0tLSBhL3NyYy9pbWFnZS5jCisrKyBiL3NyYy9pbWFnZS5jCkBAIC04NDg3LDYg Kzg0ODcsOSBAQCBnaWZfbG9hZCAoc3RydWN0IGZyYW1lICpmLCBzdHJ1Y3QgaW1hZ2UgKmltZykK ICAgICAgIH0KICAgfQogCisgIEFVVE9fU1RSSU5HIChtc2csICIjIyMjIGdpZl9sb2FkPSVzICgl cykiKTsKKyAgQ0FMTE4gKEZtZXNzYWdlLCBtc2csIG1ha2VfZml4bnVtIChpZHgpLCBGaW1hZ2Vf Y2FjaGVfc2l6ZSAoKSk7CisKICAgd2lkdGggPSBpbWctPndpZHRoID0gZ2lmLT5TV2lkdGg7CiAg IGhlaWdodCA9IGltZy0+aGVpZ2h0ID0gZ2lmLT5TSGVpZ2h0OwogCi0tIAoyLjMwLjIKCg== --000000000000e6285305cf9c58bc--