bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

[Leo Liu]

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


Switching from html-mode I have noticed mhtml-mode can take a second or
two (unpleasant noticeable delay) to load on my 2013 macbook air. The
following is a small patch that alleviate the problem. Is it safe for
emacs-26?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mhtml.diff --]
[-- Type: text/x-patch, Size: 1955 bytes --]

diff --git a/lisp/textmodes/mhtml-mode.el b/lisp/textmodes/mhtml-mode.el
index b99f7881..2ad3c8eb 100644
--- a/lisp/textmodes/mhtml-mode.el
+++ b/lisp/textmodes/mhtml-mode.el
@@ -21,13 +21,9 @@
 
 ;;; Code:
 
-(eval-and-compile
-  (require 'flyspell)
-  (require 'sgml-mode))
+(eval-and-compile (require 'sgml-mode))
 (require 'js)
 (require 'css-mode)
-(require 'prog-mode)
-(require 'font-lock)
 
 (defcustom mhtml-tag-relative-indent t
   "How <script> and <style> bodies are indented relative to the tag.
@@ -349,6 +345,8 @@ This is used by `mhtml--pre-command'.")
       ;; HTML.
       (sgml-indent-line))))
 
+(declare-function flyspell-generic-progmode-verify "flyspell")
+
 (defun mhtml--flyspell-check-word ()
   (let ((submode (get-text-property (point) 'mhtml-submode)))
     (if submode
diff --git a/lisp/textmodes/sgml-mode.el b/lisp/textmodes/sgml-mode.el
index eb6ebf52..ad3357e0 100644
--- a/lisp/textmodes/sgml-mode.el
+++ b/lisp/textmodes/sgml-mode.el
@@ -33,10 +33,9 @@
 ;;; Code:
 
 (require 'dom)
-(require 'seq)
-(eval-when-compile (require 'subr-x))
 (eval-when-compile
   (require 'skeleton)
+  (require 'subr-x)
   (require 'cl-lib))
 
 (defgroup sgml nil
@@ -2240,7 +2239,7 @@ The result is cached in `html--buffer-classes-cache'."
         (cdr html--buffer-classes-cache)
       (let* ((dom (libxml-parse-html-region (point-min) (point-max)))
              (classes
-              (seq-mapcat
+              (mapcan
                (lambda (el)
                  (when-let* ((class-list
                               (cdr (assq 'class (dom-attributes el)))))
@@ -2258,7 +2257,7 @@ The result is cached in `html--buffer-ids-cache'."
       (let* ((dom
               (libxml-parse-html-region (point-min) (point-max)))
              (ids
-              (seq-mapcat
+              (mapcan
                (lambda (el)
                  (when-let* ((id-list
                               (cdr (assq 'id (dom-attributes el)))))

bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

[Eli Zaretskii]

> From: Leo Liu <sdl.web@gmail.com>
> Date: Tue, 01 Jan 2019 10:12:05 +0800
> 
> Switching from html-mode I have noticed mhtml-mode can take a second or
> two (unpleasant noticeable delay) to load on my 2013 macbook air. The
> following is a small patch that alleviate the problem. Is it safe for
> emacs-26?

I cannot establish whether these changes are safe for Emacs 26.2,
since they are not really trivial.  Perhaps if you told more about
each change, I could make up my mind.

Some of the questions I would like to be able to answer in order to
make the decision:

  . which of the 'require's you want to remove take the lion's share
    of the load time?
  . why was seq-mapcat used originally instead of mapcan, and what is
    the semantics of replacing the former by the latter?
  . why did you switch the order of eval-when-compile in sgml-mode.el?

Thanks.

bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

[Leo Liu]

On 2019-01-05 11:47 +0200, Eli Zaretskii wrote:
> I cannot establish whether these changes are safe for Emacs 26.2,
> since they are not really trivial.  Perhaps if you told more about
> each change, I could make up my mind.

Fair.

> Some of the questions I would like to be able to answer in order to
> make the decision:
>
>   . which of the 'require's you want to remove take the lion's share
>     of the load time?

flyspell which saves ~7000 line of code.

>   . why was seq-mapcat used originally instead of mapcan, and what is
>     the semantics of replacing the former by the latter?

I am not entirely sure why it was used in the first place. Maybe people
now learns about seq.el first.

seq-mapcat doesn't mutate its args but in both cases fresh lists are
created using split-string so mapcan is safe as well i.e. the semantics
of html-current-buffer-ids and html-current-buffer-classes are intact.

I have also tested them before and after in a large HTML buffer.

>   . why did you switch the order of eval-when-compile in sgml-mode.el?

Totally cosmetic. We could keep the original order.

Thanks,
Leo

bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

[Eli Zaretskii]

> From:  Leo Liu <sdl.web@gmail.com>
> Cc: 33939@debbugs.gnu.org
> Date: Sun, 06 Jan 2019 12:34:26 +0800
> 
> >   . which of the 'require's you want to remove take the lion's share
> >     of the load time?
> 
> flyspell which saves ~7000 line of code.

Using declare-function instead of that is a no-brainer, which is good.

> >   . why was seq-mapcat used originally instead of mapcan, and what is
> >     the semantics of replacing the former by the latter?
> 
> I am not entirely sure why it was used in the first place. Maybe people
> now learns about seq.el first.
> 
> seq-mapcat doesn't mutate its args but in both cases fresh lists are
> created using split-string so mapcan is safe as well i.e. the semantics
> of html-current-buffer-ids and html-current-buffer-classes are intact.
> 
> I have also tested them before and after in a large HTML buffer.
> 
> >   . why did you switch the order of eval-when-compile in sgml-mode.el?
> 
> Totally cosmetic. We could keep the original order.

OK.  Would you be okay with doing only the first of these,
i.e. avoiding to load flyspell, on the emacs-26 branch?  Or is the use
of seq.el still slowing down the load significantly?

Thanks.

bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

[Leo Liu]

Fixed in 26.2.

On 2019-01-06 17:25 +0200, Eli Zaretskii wrote:
> OK.  Would you be okay with doing only the first of these,
> i.e. avoiding to load flyspell, on the emacs-26 branch?  Or is the use
> of seq.el still slowing down the load significantly?

Sounds good. I just did that which shaves off ~100ms.

(require 'mhtml-mode) takes ~1.5 seconds on my machine. I think the bulk
of the time is spent loading js.el which loads cc-mode but delay loading
js is trickier so I'll leave it off for now.

Thanks for reviewing the bug.

Leo

bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

[Leo Liu]

On 2019-01-06 17:25 +0200, Eli Zaretskii wrote:
> OK.  Would you be okay with doing only the first of these,
> i.e. avoiding to load flyspell, on the emacs-26 branch?  Or is the use
> of seq.el still slowing down the load significantly?

I just discovered that css-mode.el also unnecessarily loads eww which in
turn loads gazillion of other things (including gnus).

Do you mind if I also remove (require 'eww) from css-mode.el? This
shaves off another 800ms making (require 'mhtml-mode) at a more
reasonable 650ms. Sound like a win ;)

Leo

bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

[Eli Zaretskii]

> From:  Leo Liu <sdl.web@gmail.com>
> Cc: 33939@debbugs.gnu.org
> Date: Mon, 07 Jan 2019 08:33:43 +0800
> 
> I just discovered that css-mode.el also unnecessarily loads eww which in
> turn loads gazillion of other things (including gnus).
> 
> Do you mind if I also remove (require 'eww) from css-mode.el? This
> shaves off another 800ms making (require 'mhtml-mode) at a more
> reasonable 650ms. Sound like a win ;)

I don't mind, of course, but if you want this to go to the release
branch, please show the proposed patch.

Thanks.

bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

[Leo Liu]

On 2019-01-07 05:37 +0200, Eli Zaretskii wrote:
> I don't mind, of course, but if you want this to go to the release
> branch, please show the proposed patch.
>
> Thanks.

Should have attached it in the first place. This single line change
delivers the most speed up.

diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
index 016f0e8f..67a0c9f7 100644
--- a/lisp/textmodes/css-mode.el
+++ b/lisp/textmodes/css-mode.el
@@ -32,7 +32,6 @@
 
 ;;; Code:
 
-(require 'eww)
 (require 'cl-lib)
 (require 'color)
 (require 'seq)

bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

[Eli Zaretskii]

> From:  Leo Liu <sdl.web@gmail.com>
> Cc: 33939@debbugs.gnu.org
> Date: Mon, 07 Jan 2019 12:16:39 +0800
> 
> On 2019-01-07 05:37 +0200, Eli Zaretskii wrote:
> > I don't mind, of course, but if you want this to go to the release
> > branch, please show the proposed patch.
> >
> > Thanks.
> 
> Should have attached it in the first place. This single line change
> delivers the most speed up.
> 
> diff --git a/lisp/textmodes/css-mode.el b/lisp/textmodes/css-mode.el
> index 016f0e8f..67a0c9f7 100644
> --- a/lisp/textmodes/css-mode.el
> +++ b/lisp/textmodes/css-mode.el
> @@ -32,7 +32,6 @@
>  
>  ;;; Code:
>  
> -(require 'eww)
>  (require 'cl-lib)
>  (require 'color)
>  (require 'seq)
> 

This is OK for the emacs-26 branch, thanks.

bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

[Leo Liu]

On 2019-01-07 21:56 +0200, Eli Zaretskii wrote:
> This is OK for the emacs-26 branch, thanks.

Done.

bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

[Leo Liu]

On 2019-01-07 21:56 +0200, Eli Zaretskii wrote:
> This is OK for the emacs-26 branch, thanks.

I was bored enough to check if js.el had any low-hanging fruits and
found another one. js.el uses nothing from thingatpt since
forward-symbol now lives in subr.el. Should I go ahead and put the
following patch on emacs-26?

Sorry to discover these issues one after another. The situation is worse
than I imagined.

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 65ffb0e0..1e2c3411 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -48,10 +48,9 @@
 
 (require 'cc-mode)
 (require 'newcomment)
-(require 'thingatpt)                    ; forward-symbol etc
 (require 'imenu)
 (require 'moz nil t)
-(require 'json nil t)
+(require 'json)
 (require 'sgml-mode)
 (require 'prog-mode)
 

bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

[Eli Zaretskii]

> From:  Leo Liu <sdl.web@gmail.com>
> Cc: 33939@debbugs.gnu.org
> Date: Tue, 08 Jan 2019 06:43:20 +0800
> 
> On 2019-01-07 21:56 +0200, Eli Zaretskii wrote:
> > This is OK for the emacs-26 branch, thanks.
> 
> I was bored enough to check if js.el had any low-hanging fruits and
> found another one. js.el uses nothing from thingatpt since
> forward-symbol now lives in subr.el. Should I go ahead and put the
> following patch on emacs-26?

If this causes a significant slowdown of loading, yes, please.  If the
slowdown is not significant, please do this on master.

Thanks.

> Sorry to discover these issues one after another.

No need to be sorry, thanks for looking at these issues.

bug#33939: 26.1; Avoid loading libs eagerly or unnecessarily in mhtml-mode/sgml-mode

[Leo Liu]

On 2019-01-08 05:31 +0200, Eli Zaretskii wrote:
> If this causes a significant slowdown of loading, yes, please.  If the
> slowdown is not significant, please do this on master.

Installed on the master branch. Thanks.