GNU bug report logs - #28402
25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 28402 in the body.
You can then email your comments to 28402 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Cc: larsi <at> gnus.org
Subject: 25.2;
 shr.el uses shr-tag-img despite set shr-external-rendering-functions
Date: Sat, 9 Sep 2017 21:39:39 +0200

I'm using shr.el for a package and override `shr-tag-img` by let-binding
`shr-external-rendering-functions` to an alist containing an alternative
rendering function that deals better with local images.  Despite this
`shr-tag-img` is still directly called when rendering tables with images
inside them.  The same problem applies to other directly called
rendering functions, as opposed to indirectly called ones as seen in
`shr-descend`, the function honoring `shr-external-rendering-functions`.

Message #8 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
Cc: larsi <at> gnus.org, 28402 <at> debbugs.gnu.org
Subject: Re: bug#28402: 25.2;
 shr.el uses shr-tag-img despite set shr-external-rendering-functions
Date: Mon, 11 Sep 2017 19:04:42 +0300

> From: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
> Date: Sat, 9 Sep 2017 21:39:39 +0200
> Cc: larsi <at> gnus.org
> 
> I'm using shr.el for a package and override `shr-tag-img` by let-binding
> `shr-external-rendering-functions` to an alist containing an alternative
> rendering function that deals better with local images.  Despite this
> `shr-tag-img` is still directly called when rendering tables with images
> inside them.  The same problem applies to other directly called
> rendering functions, as opposed to indirectly called ones as seen in
> `shr-descend`, the function honoring `shr-external-rendering-functions`.

From a cursory look into shr.el, it looks like
shr-external-rendering-functions were introduced to support some
specific eww.el needs, not as a general lever for tweaking shr.el from
outside it.  That's why only a handful of functions honor that
variable.

Would you like to submit a patch that would make all shr-tag-*
functions consult that variable first?

Message #11 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, 28402 <at> debbugs.gnu.org
Subject: Re: bug#28402: 25.2; shr.el uses shr-tag-img despite set
 shr-external-rendering-functions
Date: Wed, 13 Sep 2017 19:22:32 +0200

> From a cursory look into shr.el, it looks like
> shr-external-rendering-functions were introduced to support some
> specific eww.el needs, not as a general lever for tweaking shr.el from
> outside it.  That's why only a handful of functions honor that
> variable.

I suspected as much.

> Would you like to submit a patch that would make all shr-tag-*
> functions consult that variable first?

Before doing that I'd like to get a confirmation from the author whether
it's intended for shr-external-rendering-functions to become public API
and whether it should work globally, hence why I included them in the
CC.

Message #14 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
Cc: 28402 <at> debbugs.gnu.org
Subject: Re: bug#28402: 25.2;
 shr.el uses shr-tag-img despite set shr-external-rendering-functions
Date: Wed, 13 Sep 2017 19:27:29 +0200

Vasilij Schneidermann <v.schneidermann <at> gmail.com> writes:

> I'm using shr.el for a package and override `shr-tag-img` by let-binding
> `shr-external-rendering-functions` to an alist containing an alternative
> rendering function that deals better with local images.  Despite this
> `shr-tag-img` is still directly called when rendering tables with images
> inside them. 

I think it would be nice to alter those direct calls to heed
`shr-external-rendering-functions'.  Doesn't seem to be much potential
for performance degradation that I can see...

Perhaps introduce a helper function like
`(shr-indirect-call 'img child)' or something...

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

Message #17 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
Cc: larsi <at> gnus.org, 28402 <at> debbugs.gnu.org
Subject: Re: bug#28402: 25.2; shr.el uses shr-tag-img despite set
 shr-external-rendering-functions
Date: Wed, 13 Sep 2017 21:37:17 +0300

> Date: Wed, 13 Sep 2017 19:22:32 +0200
> From: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
> Cc: 28402 <at> debbugs.gnu.org, larsi <at> gnus.org
> 
> > Would you like to submit a patch that would make all shr-tag-*
> > functions consult that variable first?
> 
> Before doing that I'd like to get a confirmation from the author whether
> it's intended for shr-external-rendering-functions to become public API
> and whether it should work globally, hence why I included them in the
> CC.

Fair enough.  Fortunately, it sounds like Lars have just agreed to
that.

Thanks.

Message #20 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: larsi <at> gnus.org, 28402 <at> debbugs.gnu.org
Subject: Re: bug#28402: 25.2; shr.el uses shr-tag-img despite set
 shr-external-rendering-functions
Date: Sun, 24 Sep 2017 15:10:50 +0200

[Message part 1 (text/plain, inline)]

> Fair enough.  Fortunately, it sounds like Lars have just agreed to
> that.

Alright, I finally got to creating a patch for this.

[0001-Add-indirection-to-all-shr-tag-calls.patch (text/x-diff, attachment)]

Message #23 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
Cc: larsi <at> gnus.org, 28402 <at> debbugs.gnu.org
Subject: Re: bug#28402: 25.2; shr.el uses shr-tag-img despite set
 shr-external-rendering-functions
Date: Fri, 29 Sep 2017 10:31:15 +0300

> Date: Sun, 24 Sep 2017 15:10:50 +0200
> From: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
> Cc: 28402 <at> debbugs.gnu.org, larsi <at> gnus.org
> 
> > Fair enough.  Fortunately, it sounds like Lars have just agreed to
> > that.
> 
> Alright, I finally got to creating a patch for this.

Thanks, this LGTM.  Lars, any comments?

If no objections are voiced in a few days, I will push to the emacs-26
branch.

Message #28 received at 28402-done <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: v.schneidermann <at> gmail.com, larsi <at> gnus.org
Cc: 28402-done <at> debbugs.gnu.org
Subject: Re: bug#28402: 25.2;
 shr.el uses shr-tag-img despite set shr-external-rendering-functions
Date: Thu, 05 Oct 2017 13:02:27 +0300

> Date: Fri, 29 Sep 2017 10:31:15 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: larsi <at> gnus.org, 28402 <at> debbugs.gnu.org
> 
> If no objections are voiced in a few days, I will push to the emacs-26
> branch.

Done.

Message #31 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28402 <at> debbugs.gnu.org, Vasilij Schneidermann <v.schneidermann <at> gmail.com>
Subject: Re: bug#28402: 25.2;
 shr.el uses shr-tag-img despite set shr-external-rendering-functions
Date: Thu, 05 Oct 2017 12:18:40 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

> If no objections are voiced in a few days, I will push to the emacs-26
> branch.

Sorry; I didn't have a look at this before you applied.

It mostly looks fine, but this bit isn't:

 (defun shr-descend (dom)
-  (let ((function
-         (intern (concat "shr-tag-" (symbol-name (dom-tag dom))) obarray))
-        ;; Allow other packages to override (or provide) rendering
-        ;; of elements.
-        (external (cdr (assq (dom-tag dom) shr-external-rendering-functions)))
+  (let ((tag-name (dom-tag dom))
 	(style (dom-attr dom 'style))
 	(shr-stylesheet shr-stylesheet)
 	(shr-depth (1+ shr-depth))
@@ -490,12 +498,7 @@ shr-descend
 	  (setq style nil)))
       ;; If we have a display:none, then just ignore this part of the DOM.
       (unless (equal (cdr (assq 'display shr-stylesheet)) "none")
-        (cond (external
-               (funcall external dom))
-              ((fboundp function)
-               (funcall function dom))
-              (t
-               (shr-generic dom)))
+	(shr-indirect-call tag-name dom)

shr rendering of deep HTML structures uses a lot of stack, and we see
this in practice sometimes, where shr refuses to render HTML because
it's too deeply nested (and runs into the Emacs max-depth stack thing).

This indirect call will make the stack 30% deeper, I think?  As well as
slower, since it's an extra funcall for each and every HTML node.

So this part should be reverted.

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

Message #34 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 28402 <at> debbugs.gnu.org, v.schneidermann <at> gmail.com
Subject: Re: bug#28402: 25.2;
 shr.el uses shr-tag-img despite set shr-external-rendering-functions
Date: Thu, 05 Oct 2017 14:01:33 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: Vasilij Schneidermann <v.schneidermann <at> gmail.com>,  28402 <at> debbugs.gnu.org
> Date: Thu, 05 Oct 2017 12:18:40 +0200
> 
>  (defun shr-descend (dom)
> -  (let ((function
> -         (intern (concat "shr-tag-" (symbol-name (dom-tag dom))) obarray))
> -        ;; Allow other packages to override (or provide) rendering
> -        ;; of elements.
> -        (external (cdr (assq (dom-tag dom) shr-external-rendering-functions)))
> +  (let ((tag-name (dom-tag dom))
>  	(style (dom-attr dom 'style))
>  	(shr-stylesheet shr-stylesheet)
>  	(shr-depth (1+ shr-depth))
> @@ -490,12 +498,7 @@ shr-descend
>  	  (setq style nil)))
>        ;; If we have a display:none, then just ignore this part of the DOM.
>        (unless (equal (cdr (assq 'display shr-stylesheet)) "none")
> -        (cond (external
> -               (funcall external dom))
> -              ((fboundp function)
> -               (funcall function dom))
> -              (t
> -               (shr-generic dom)))
> +	(shr-indirect-call tag-name dom)
> 
> shr rendering of deep HTML structures uses a lot of stack, and we see
> this in practice sometimes, where shr refuses to render HTML because
> it's too deeply nested (and runs into the Emacs max-depth stack thing).
> 
> This indirect call will make the stack 30% deeper, I think?  As well as
> slower, since it's an extra funcall for each and every HTML node.
> 
> So this part should be reverted.

Wouldn't reverting it make the entire change rather pointless?

If the issue is with stack usage, we could increase the stack when
shr-descend is called.  As for speed, the old code had such an
indirection as well, albeit via funcall, no?

So I don't think I understand the nature of your objections.

Message #37 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28402 <at> debbugs.gnu.org, v.schneidermann <at> gmail.com
Subject: Re: bug#28402: 25.2;
 shr.el uses shr-tag-img despite set shr-external-rendering-functions
Date: Thu, 05 Oct 2017 13:18:37 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

>> -        (cond (external
>> -               (funcall external dom))
>> -              ((fboundp function)
>> -               (funcall function dom))
>> -              (t
>> -               (shr-generic dom)))
>> +	(shr-indirect-call tag-name dom)

[...]

> Wouldn't reverting it make the entire change rather pointless?

No, the other calls to shr-indirect are fine, and was what was missing.

> If the issue is with stack usage, we could increase the stack when
> shr-descend is called.  As for speed, the old code had such an
> indirection as well, albeit via funcall, no?

The old call calls virtually always ends up with

(funcall function dom)

The new code calls shr-indirect-call which will will then 

(funcall function dom)

So one extra function call for each node.

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

Message #40 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 28402 <at> debbugs.gnu.org, v.schneidermann <at> gmail.com
Subject: Re: bug#28402: 25.2;
 shr.el uses shr-tag-img despite set shr-external-rendering-functions
Date: Thu, 05 Oct 2017 16:14:25 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: v.schneidermann <at> gmail.com,  28402 <at> debbugs.gnu.org
> Date: Thu, 05 Oct 2017 13:18:37 +0200
> 
> The old call calls virtually always ends up with
> 
> (funcall function dom)
> 
> The new code calls shr-indirect-call which will will then 
> 
> (funcall function dom)
> 
> So one extra function call for each node.

What if we make shr-indirect-call a defsubst?  Would that take care of
this issue?

Message #43 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28402 <at> debbugs.gnu.org, v.schneidermann <at> gmail.com
Subject: Re: bug#28402: 25.2;
 shr.el uses shr-tag-img despite set shr-external-rendering-functions
Date: Thu, 05 Oct 2017 15:22:46 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

> What if we make shr-indirect-call a defsubst?  Would that take care of
> this issue?

Isn't it still (at least) an extra stack frame with a bunch of variable
bindings?

This function is the central bit in shr, and should be as fast as
possible.

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

Message #46 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 28402 <at> debbugs.gnu.org, v.schneidermann <at> gmail.com
Subject: Re: bug#28402: 25.2;
 shr.el uses shr-tag-img despite set shr-external-rendering-functions
Date: Thu, 05 Oct 2017 16:44:57 +0300

> From: Lars Ingebrigtsen <larsi <at> gnus.org>
> Cc: v.schneidermann <at> gmail.com,  28402 <at> debbugs.gnu.org
> Date: Thu, 05 Oct 2017 15:22:46 +0200
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> > What if we make shr-indirect-call a defsubst?  Would that take care of
> > this issue?
> 
> Isn't it still (at least) an extra stack frame with a bunch of variable
> bindings?

Yes, but is that really that significant?  You sound like saying that
any non-trivial change in shr-descend should be rejected for these
reasons.  Is that really so?  Do we have measurements that would back
up such extreme care?

> This function is the central bit in shr, and should be as fast as
> possible.

As fast as possible, but not faster, I presume ;-)

Anyway, I see your point.  Vasilij, any thoughts or suggestions to
avoid reverting that part, while still keeping Lars happy?

Message #49 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 28402 <at> debbugs.gnu.org, v.schneidermann <at> gmail.com
Subject: Re: bug#28402: 25.2;
 shr.el uses shr-tag-img despite set shr-external-rendering-functions
Date: Thu, 05 Oct 2017 15:52:23 +0200

Eli Zaretskii <eliz <at> gnu.org> writes:

> Yes, but is that really that significant?  You sound like saying that
> any non-trivial change in shr-descend should be rejected for these
> reasons.  Is that really so?  Do we have measurements that would back
> up such extreme care?

No, I'm just saying that any additional features added to that function
should be considered carefully (to see whether that added feature is
worth the performance degradation).  And I don't think we should do
anything to "pretty it up" just because, if that has any performance
impact at all.

In a thing like shr, it's really the case of a death by a thousand cuts:
Each single improvement adds a slight performance hit, and then after a
couple of years you end up with something that's pretty, but completely
unusable.  (It's already too slow as it is.)  So I protect `shr-descend'
fiercely.  :-)

(But late, as always...  Sorry for not saying this before you applied
the patch; it would have been less work for all of us.)

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

Message #52 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 28402 <at> debbugs.gnu.org
Subject: Re: bug#28402: 25.2; shr.el uses shr-tag-img despite set
 shr-external-rendering-functions
Date: Thu, 5 Oct 2017 22:08:07 +0200

> In a thing like shr, it's really the case of a death by a thousand cuts:
> Each single improvement adds a slight performance hit, and then after a
> couple of years you end up with something that's pretty, but completely
> unusable.  (It's already too slow as it is.)  So I protect `shr-descend'
> fiercely.  :-)

How deeply nested does typical HTML get anyways?  I recall an exemplary
comment from an article on browser rendering that pointed out that they
bail out on degenerate cases, so perhaps it's better to add that to shr
rather than hoping you never hit the stack limit by following a
conservative coding style.

Regarding speed, I only notice it being an issue for documents with
tables, everything else is fine.

> (But late, as always...  Sorry for not saying this before you applied
> the patch; it would have been less work for all of us.)

I can imagine other variations of defsubst (such as defmacro and
define-inline), but it would indeed be the easiest to roll back that
part of the code.  What I'd additionally do is adding a comment
explaining why there's two very similar snippets of code doing the same
thing, otherwise the next person proposing a patch will change only one
of them and be left head-scratching why it doesn't quite work.

Message #55 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 28402 <at> debbugs.gnu.org
Subject: Re: bug#28402: 25.2;
 shr.el uses shr-tag-img despite set shr-external-rendering-functions
Date: Thu, 05 Oct 2017 22:10:52 +0200

Vasilij Schneidermann <v.schneidermann <at> gmail.com> writes:

> How deeply nested does typical HTML get anyways?

Very.

> I recall an exemplary comment from an article on browser rendering
> that pointed out that they bail out on degenerate cases, so perhaps
> it's better to add that to shr rather than hoping you never hit the
> stack limit by following a conservative coding style.

shr does check and bail.  Search for max-specpdl-size in shr.el.

> What I'd additionally do is adding a comment explaining why there's
> two very similar snippets of code doing the same thing, otherwise the
> next person proposing a patch will change only one of them and be left
> head-scratching why it doesn't quite work.

Sure; please submit a patch to do so.

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

Message #58 received at 28402 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
Cc: larsi <at> gnus.org, 28402 <at> debbugs.gnu.org
Subject: Re: bug#28402: 25.2; shr.el uses shr-tag-img despite set
 shr-external-rendering-functions
Date: Fri, 06 Oct 2017 15:43:49 +0300

> Date: Thu, 5 Oct 2017 22:08:07 +0200
> From: Vasilij Schneidermann <v.schneidermann <at> gmail.com>
> Cc: Eli Zaretskii <eliz <at> gnu.org>, 28402 <at> debbugs.gnu.org
> 
> I can imagine other variations of defsubst (such as defmacro and
> define-inline), but it would indeed be the easiest to roll back that
> part of the code.  What I'd additionally do is adding a comment
> explaining why there's two very similar snippets of code doing the same
> thing, otherwise the next person proposing a patch will change only one
> of them and be left head-scratching why it doesn't quite work.

Thanks, done.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.