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:50:19 -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="0000000000006902b705cf9c487b" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="4317"; 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:51:13 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 1mh008-0000tD-Lk for geb-bug-gnu-emacs@m.gmane-mx.org; Sun, 31 Oct 2021 02:51:12 +0100 Original-Received: from localhost ([::1]:56308 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mh006-0000pj-It for geb-bug-gnu-emacs@m.gmane-mx.org; Sat, 30 Oct 2021 21:51:10 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:46730) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mgzzz-0000pZ-A8 for bug-gnu-emacs@gnu.org; Sat, 30 Oct 2021 21:51:03 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:47157) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mgzzy-0006nJ-23 for bug-gnu-emacs@gnu.org; Sat, 30 Oct 2021 21:51:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1mgzzx-00012b-WF for bug-gnu-emacs@gnu.org; Sat, 30 Oct 2021 21:51:02 -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:51: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.16356450283961 (code B ref 45224); Sun, 31 Oct 2021 01:51:01 +0000 Original-Received: (at 45224) by debbugs.gnu.org; 31 Oct 2021 01:50:28 +0000 Original-Received: from localhost ([127.0.0.1]:58703 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mgzzQ-00011p-4B for submit@debbugs.gnu.org; Sat, 30 Oct 2021 21:50:28 -0400 Original-Received: from mail-pg1-f180.google.com ([209.85.215.180]:38410) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1mgzzO-00011c-4k for 45224@debbugs.gnu.org; Sat, 30 Oct 2021 21:50:26 -0400 Original-Received: by mail-pg1-f180.google.com with SMTP id e65so13768328pgc.5 for <45224@debbugs.gnu.org>; Sat, 30 Oct 2021 18:50:26 -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=RKw7GhXfxnW/cvJIT7QSUuvmWdUpWphEpafO2FbjQoo=; b=CNwSZ9X2O1HUJ11FcZortv7DGbZ2KDb0GouED9p34jeA/vuHHTIp2gzl1XiaQnrNeO TLVSZX1E7MjZXPYugzVOchKwbRglfbw1rXd6HQcRooSuvvdSINdjZxKHpvBhi+lddy8s 8zuOTOzB6IkgANSYTEbL6ygla6F3PuGJFnIk9s3AqHQNKam732P4H6EimOkIrz/amwur NsL5M2BdypYMSSei0TBh9C634BOTHOx4yvpeEBeY548AMksck7twgaI679YOz547Mn4W k48JxQ4RLVQP/us5Q0m1sPm9pqBOBcAG54T/gGJH4vkDlR3mwutJIAbs8rtw1nXtZnnh HbFQ== X-Gm-Message-State: AOAM530i3JuJBO469rv3GJxKQuzU4d7wvLk5egd46gX3Dx3I/FxoQ/17 N4cGeT3CUxmBStoV6BlYEb31dLef7zvjYi5rnls= X-Google-Smtp-Source: ABdhPJw82VQrKEDsxAhW2CZg3bysoNH/V/CKk6pjHw0FitxYX1nH9M3fDd3YtEtHU+/xQPN7ih5AXxdkcMzLYqWBYog= X-Received: by 2002:a63:370c:: with SMTP id e12mr15020740pga.359.1635645020234; Sat, 30 Oct 2021 18:50:20 -0700 (PDT) Original-Received: from 753933720722 named unknown by gmailapi.google.com with HTTPREST; Sat, 30 Oct 2021 18:50:19 -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:218647 Archived-At: --0000000000006902b705cf9c487b 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 ones. 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. I can think of two main ways to do 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 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! 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. --0000000000006902b705cf9c487b 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: b573af277b81d00_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== --0000000000006902b705cf9c487b--