unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
@ 2017-09-09 19:39 Vasilij Schneidermann
  2017-09-11 16:04 ` Eli Zaretskii
  2017-09-13 17:27 ` Lars Ingebrigtsen
  0 siblings, 2 replies; 18+ messages in thread
From: Vasilij Schneidermann @ 2017-09-09 19:39 UTC (permalink / raw)
  To: 28402; +Cc: larsi

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-09-09 19:39 bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions Vasilij Schneidermann
@ 2017-09-11 16:04 ` Eli Zaretskii
  2017-09-13 17:22   ` Vasilij Schneidermann
  2017-09-13 17:27 ` Lars Ingebrigtsen
  1 sibling, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2017-09-11 16:04 UTC (permalink / raw)
  To: Vasilij Schneidermann; +Cc: larsi, 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-09-11 16:04 ` Eli Zaretskii
@ 2017-09-13 17:22   ` Vasilij Schneidermann
  2017-09-13 18:37     ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Vasilij Schneidermann @ 2017-09-13 17:22 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-09-09 19:39 bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions Vasilij Schneidermann
  2017-09-11 16:04 ` Eli Zaretskii
@ 2017-09-13 17:27 ` Lars Ingebrigtsen
  1 sibling, 0 replies; 18+ messages in thread
From: Lars Ingebrigtsen @ 2017-09-13 17:27 UTC (permalink / raw)
  To: Vasilij Schneidermann; +Cc: 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-09-13 17:22   ` Vasilij Schneidermann
@ 2017-09-13 18:37     ` Eli Zaretskii
  2017-09-24 13:10       ` Vasilij Schneidermann
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2017-09-13 18:37 UTC (permalink / raw)
  To: Vasilij Schneidermann; +Cc: larsi, 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-09-13 18:37     ` Eli Zaretskii
@ 2017-09-24 13:10       ` Vasilij Schneidermann
  2017-09-29  7:31         ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Vasilij Schneidermann @ 2017-09-24 13:10 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: larsi, 28402

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


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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-09-24 13:10       ` Vasilij Schneidermann
@ 2017-09-29  7:31         ` Eli Zaretskii
  2017-10-05 10:02           ` Eli Zaretskii
  2017-10-05 10:18           ` Lars Ingebrigtsen
  0 siblings, 2 replies; 18+ messages in thread
From: Eli Zaretskii @ 2017-09-29  7:31 UTC (permalink / raw)
  To: Vasilij Schneidermann; +Cc: larsi, 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-09-29  7:31         ` Eli Zaretskii
@ 2017-10-05 10:02           ` Eli Zaretskii
  2017-10-05 10:18           ` Lars Ingebrigtsen
  1 sibling, 0 replies; 18+ messages in thread
From: Eli Zaretskii @ 2017-10-05 10:02 UTC (permalink / raw)
  To: v.schneidermann, larsi; +Cc: 28402-done

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-09-29  7:31         ` Eli Zaretskii
  2017-10-05 10:02           ` Eli Zaretskii
@ 2017-10-05 10:18           ` Lars Ingebrigtsen
  2017-10-05 11:01             ` Eli Zaretskii
  1 sibling, 1 reply; 18+ messages in thread
From: Lars Ingebrigtsen @ 2017-10-05 10:18 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: Vasilij Schneidermann, 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-10-05 10:18           ` Lars Ingebrigtsen
@ 2017-10-05 11:01             ` Eli Zaretskii
  2017-10-05 11:18               ` Lars Ingebrigtsen
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2017-10-05 11:01 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: v.schneidermann, 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-10-05 11:01             ` Eli Zaretskii
@ 2017-10-05 11:18               ` Lars Ingebrigtsen
  2017-10-05 13:14                 ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Lars Ingebrigtsen @ 2017-10-05 11:18 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: v.schneidermann, 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-10-05 11:18               ` Lars Ingebrigtsen
@ 2017-10-05 13:14                 ` Eli Zaretskii
  2017-10-05 13:22                   ` Lars Ingebrigtsen
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2017-10-05 13:14 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: v.schneidermann, 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-10-05 13:14                 ` Eli Zaretskii
@ 2017-10-05 13:22                   ` Lars Ingebrigtsen
  2017-10-05 13:44                     ` Eli Zaretskii
  0 siblings, 1 reply; 18+ messages in thread
From: Lars Ingebrigtsen @ 2017-10-05 13:22 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: v.schneidermann, 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-10-05 13:22                   ` Lars Ingebrigtsen
@ 2017-10-05 13:44                     ` Eli Zaretskii
  2017-10-05 13:52                       ` Lars Ingebrigtsen
  0 siblings, 1 reply; 18+ messages in thread
From: Eli Zaretskii @ 2017-10-05 13:44 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: v.schneidermann, 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-10-05 13:44                     ` Eli Zaretskii
@ 2017-10-05 13:52                       ` Lars Ingebrigtsen
  2017-10-05 20:08                         ` Vasilij Schneidermann
  0 siblings, 1 reply; 18+ messages in thread
From: Lars Ingebrigtsen @ 2017-10-05 13:52 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: v.schneidermann, 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-10-05 13:52                       ` Lars Ingebrigtsen
@ 2017-10-05 20:08                         ` Vasilij Schneidermann
  2017-10-05 20:10                           ` Lars Ingebrigtsen
  2017-10-06 12:43                           ` Eli Zaretskii
  0 siblings, 2 replies; 18+ messages in thread
From: Vasilij Schneidermann @ 2017-10-05 20:08 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-10-05 20:08                         ` Vasilij Schneidermann
@ 2017-10-05 20:10                           ` Lars Ingebrigtsen
  2017-10-06 12:43                           ` Eli Zaretskii
  1 sibling, 0 replies; 18+ messages in thread
From: Lars Ingebrigtsen @ 2017-10-05 20:10 UTC (permalink / raw)
  To: Vasilij Schneidermann; +Cc: 28402

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





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

* bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions
  2017-10-05 20:08                         ` Vasilij Schneidermann
  2017-10-05 20:10                           ` Lars Ingebrigtsen
@ 2017-10-06 12:43                           ` Eli Zaretskii
  1 sibling, 0 replies; 18+ messages in thread
From: Eli Zaretskii @ 2017-10-06 12:43 UTC (permalink / raw)
  To: Vasilij Schneidermann; +Cc: larsi, 28402

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





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

end of thread, other threads:[~2017-10-06 12:43 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-09 19:39 bug#28402: 25.2; shr.el uses shr-tag-img despite set shr-external-rendering-functions Vasilij Schneidermann
2017-09-11 16:04 ` Eli Zaretskii
2017-09-13 17:22   ` Vasilij Schneidermann
2017-09-13 18:37     ` Eli Zaretskii
2017-09-24 13:10       ` Vasilij Schneidermann
2017-09-29  7:31         ` Eli Zaretskii
2017-10-05 10:02           ` Eli Zaretskii
2017-10-05 10:18           ` Lars Ingebrigtsen
2017-10-05 11:01             ` Eli Zaretskii
2017-10-05 11:18               ` Lars Ingebrigtsen
2017-10-05 13:14                 ` Eli Zaretskii
2017-10-05 13:22                   ` Lars Ingebrigtsen
2017-10-05 13:44                     ` Eli Zaretskii
2017-10-05 13:52                       ` Lars Ingebrigtsen
2017-10-05 20:08                         ` Vasilij Schneidermann
2017-10-05 20:10                           ` Lars Ingebrigtsen
2017-10-06 12:43                           ` Eli Zaretskii
2017-09-13 17:27 ` 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).