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

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

[Vasilij Schneidermann]

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`.

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

[Eli Zaretskii]

> From: Vasilij Schneidermann <v.schneidermann@gmail.com>
> Date: Sat, 9 Sep 2017 21:39:39 +0200
> Cc: larsi@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?

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

[Vasilij Schneidermann]

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

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

[Lars Ingebrigtsen]

Vasilij Schneidermann <v.schneidermann@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

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

[Eli Zaretskii]

> Date: Wed, 13 Sep 2017 19:22:32 +0200
> From: Vasilij Schneidermann <v.schneidermann@gmail.com>
> Cc: 28402@debbugs.gnu.org, larsi@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.

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

[Vasilij Schneidermann]

[-- Attachment #1: Type: text/plain, Size: 131 bytes --]

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

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

[-- Attachment #2: 0001-Add-indirection-to-all-shr-tag-calls.patch --]
[-- Type: text/x-diff, Size: 3625 bytes --]

From abe3f5702999b3b11645d924c7d05f48f4a265c4 Mon Sep 17 00:00:00 2001
From: Vasilij Schneidermann <mail@vasilij.de>
Date: Sun, 24 Sep 2017 14:59:10 +0200
Subject: [PATCH] Add indirection to all shr-tag-* calls

The 'shr-external-rendering-functions' variable was previously only
honored in the shr-descend function, now all direct calls to the
shr-tag-* functions have been replaced by a call to
'shr-indirect-call' which tries using an alternative rendering
function first.

* lisp/net/shr.el (shr-indirect-call): New helper function.
(shr-descend, shr-tag-object, shr-tag-video):
(shr-collect-extra-strings-in-table): Fix callers
---
 lisp/net/shr.el | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index 7af6148e47..fe5197b35f 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -470,12 +470,20 @@ shr-generic
 	(shr-insert sub)
       (shr-descend sub))))
 
+(defun shr-indirect-call (tag-name dom &rest args)
+  (let ((function (intern (concat "shr-tag-" (symbol-name tag-name)) obarray))
+	;; Allow other packages to override (or provide) rendering
+	;; of elements.
+	(external (cdr (assq tag-name shr-external-rendering-functions))))
+    (cond (external
+	   (apply external dom args))
+	  ((fboundp function)
+	   (apply function dom args))
+	  (t
+	   (apply 'shr-generic dom args)))))
+
 (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)
 	(when (and shr-target-id
 		   (equal (dom-attr dom 'id) shr-target-id))
 	  ;; If the element was empty, we don't have anything to put the
@@ -1404,7 +1407,7 @@ shr-tag-object
       (when url
 	(cond
 	 (image
-	  (shr-tag-img dom url)
+	  (shr-indirect-call 'img dom url)
 	  (setq dom nil))
 	 (multimedia
 	  (shr-insert " [multimedia] ")
@@ -1469,7 +1472,7 @@ shr-tag-video
     (unless url
       (setq url (car (shr--extract-best-source dom))))
     (if (> (length image) 0)
-        (shr-tag-img nil image)
+	(shr-indirect-call 'img nil image)
       (shr-insert " [video] "))
     (shr-urlify start (shr-expand-url url))))
 
@@ -1964,9 +1967,9 @@ shr-collect-extra-strings-in-table
 	     do (setq tag (dom-tag child)) and
 	     unless (memq tag '(comment style))
 	       if (eq tag 'img)
-		 do (shr-tag-img child)
+		 do (shr-indirect-call 'img child)
 	       else if (eq tag 'object)
-		 do (shr-tag-object child)
+		 do (shr-indirect-call 'object child)
 	       else
 		 do (setq recurse t) and
 		 if (eq tag 'tr)
@@ -1980,7 +1983,7 @@ shr-collect-extra-strings-in-table
 		     do (setq flags nil)
 		   else if (car flags)
 		     do (setq recurse nil)
-			(shr-tag-table child)
+			(shr-indirect-call 'table child)
 		   end end end end end end end end end end
 	   when recurse
 	     append (shr-collect-extra-strings-in-table child flags)))
-- 
2.14.1

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

[Eli Zaretskii]

> Date: Sun, 24 Sep 2017 15:10:50 +0200
> From: Vasilij Schneidermann <v.schneidermann@gmail.com>
> Cc: 28402@debbugs.gnu.org, larsi@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.

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

[Eli Zaretskii]

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

Done.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Vasilij Schneidermann <v.schneidermann@gmail.com>,  28402@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.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: v.schneidermann@gmail.com,  28402@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?

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: v.schneidermann@gmail.com,  28402@debbugs.gnu.org
> Date: Thu, 05 Oct 2017 15:22:46 +0200
> 
> Eli Zaretskii <eliz@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?

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@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

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

[Vasilij Schneidermann]

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

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

[Lars Ingebrigtsen]

Vasilij Schneidermann <v.schneidermann@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

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

[Eli Zaretskii]

> Date: Thu, 5 Oct 2017 22:08:07 +0200
> From: Vasilij Schneidermann <v.schneidermann@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, 28402@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.