Re: master updated (cd8d3f3379e -> 1b0348d9593)

Re: master updated (cd8d3f3379e -> 1b0348d9593)

[Michael Albinus]

Stefan Monnier via Mailing list for Emacs changes <emacs-diffs@gnu.org>
writes:

Hi,

> monnier pushed a change to branch master.
>
>       from  cd8d3f3379e Fix some tree-sitter :match regexps
>        new  f411cc3a957 * lisp/emacs-lisp/lisp-mode.el (lisp-ppss): Fix performance bug
>        new  184106be267 pp.el (pp-default-function): New custom var
>        new  2f181d60323 pp.el (pp-fill): New default pp function
>        new  017475a70ed * doc/lispref/streams.texi (Output Variables): Document `pp-default-function`
>        new  a9c962be961 pp-fill: Fix tests breakage
>        new  1b0348d9593 Merge branch 'master' of git+ssh://git.sv.gnu.org/srv/git/emacs
>
>
> Summary of changes:
>  doc/lispref/streams.texi                |   9 +
>  etc/NEWS                                |   7 +
>  lisp/emacs-lisp/lisp-mode.el            |   2 +-
>  lisp/emacs-lisp/pp.el                   | 288 ++++++++++++++++++++++++++------
>  test/lisp/emacs-lisp/backtrace-tests.el |   6 +-
>  test/lisp/emacs-lisp/pp-tests.el        |   4 +-
>  6 files changed, 260 insertions(+), 56 deletions(-)

It looks like this patch series has broken esh-util-tests.el. Try

--8<---------------cut here---------------start------------->8---
# make -C test esh-util-tests
...
Test esh-util-test/eshell-stringify/list condition:
    (ert-test-failed
     ((should (equal (eshell-stringify ...) "((1 2)\n (3 . 4))")) :form
      (equal "((1 2) (3 . 4))" "((1 2)\n (3 . 4))") :value nil
      :explanation
      (arrays-of-different-length 15 16 "((1 2) (3 . 4))"
				  "((1 2)\n (3 . 4))" first-mismatch-at 6)))
   FAILED   9/13  esh-util-test/eshell-stringify/list (0.000212 sec) at lisp/eshell/esh-util-tests.el:47
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

Re: master updated (cd8d3f3379e -> 1b0348d9593)

[Stefan Monnier]

> It looks like this patch series has broken esh-util-tests.el. Try
>
> --8<---------------cut here---------------start------------->8---
> # make -C test esh-util-tests
> ...
> Test esh-util-test/eshell-stringify/list condition:
>     (ert-test-failed
>      ((should (equal (eshell-stringify ...) "((1 2)\n (3 . 4))")) :form
>       (equal "((1 2) (3 . 4))" "((1 2)\n (3 . 4))") :value nil
>       :explanation
>       (arrays-of-different-length 15 16 "((1 2) (3 . 4))"
> 				  "((1 2)\n (3 . 4))" first-mismatch-at 6)))
>    FAILED   9/13  esh-util-test/eshell-stringify/list (0.000212 sec) at lisp/eshell/esh-util-tests.el:47
> --8<---------------cut here---------------end--------------->8---

I don't think it's a bug if the output of the pretty printer is

    ((1 2) (3 . 4))

rather than

    ((1 2)
     (3 . 4))

so I think the esh-util-tests is just too strict: both output are valid,
as are various others such as

    ((1
      2)
     (3
      . 4))

Not knowing what the test is indeed the check I'm not sure how it should
be fixed.  Maybe replace all `[ \n]+` with SPC ?


        Stefan

Re: master updated (cd8d3f3379e -> 1b0348d9593)

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Sun, 18 Jun 2023 14:28:00 -0400
> 
> Not knowing what the test is indeed the check I'm not sure how it should
> be fixed.  Maybe replace all `[ \n]+` with SPC ?

Indeed, that's what I would do.  Or maybe read the result back and
compare Lisp objects?

Re: master updated (cd8d3f3379e -> 1b0348d9593)

[Eli Zaretskii]

> Date: Sun, 18 Jun 2023 21:30:20 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: michael.albinus@gmx.de, emacs-devel@gnu.org
> 
> > From: Stefan Monnier <monnier@iro.umontreal.ca>
> > Cc: emacs-devel@gnu.org
> > Date: Sun, 18 Jun 2023 14:28:00 -0400
> > 
> > Not knowing what the test is indeed the check I'm not sure how it should
> > be fixed.  Maybe replace all `[ \n]+` with SPC ?
> 
> Indeed, that's what I would do.  Or maybe read the result back and
> compare Lisp objects?

Jim, could you please look into fixing the test?

Thanks.

Re: master updated (cd8d3f3379e -> 1b0348d9593)

[Jim Porter]

On 6/18/2023 11:39 AM, Eli Zaretskii wrote:
>> Date: Sun, 18 Jun 2023 21:30:20 +0300
>> From: Eli Zaretskii <eliz@gnu.org>
>> Cc: michael.albinus@gmx.de, emacs-devel@gnu.org
>>
>>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>>> Cc: emacs-devel@gnu.org
>>> Date: Sun, 18 Jun 2023 14:28:00 -0400
>>>
>>> Not knowing what the test is indeed the check I'm not sure how it should
>>> be fixed.  Maybe replace all `[ \n]+` with SPC ?
>>
[snip]
> 
> Jim, could you please look into fixing the test?

Thanks for letting me know. I've fixed this as suggested above, and 
added a comment explaining what the test should, well... test.

Re: master 2f181d60323 3/6: pp.el (pp-fill): New default pp function

[Michael Heerdegen]

Stefan Monnier via Mailing list for Emacs changes <emacs-diffs@gnu.org>
writes:

> branch: master
> commit 2f181d60323bd9e0196775828de633100479f4c2
> Author: Stefan Monnier <monnier@iro.umontreal.ca>
> Commit: Stefan Monnier <monnier@iro.umontreal.ca>
>
>     pp.el (pp-fill): New default pp function
> [...]
> +            (when (and paired (not (eq ?\" (char-after paired))))
> +              ;; The sexp has sub-parts, so let's try and spread
> +              ;; them over several lines.
> +              (save-excursion
> +                (goto-char beg)
> +                (when (looking-at "(\\([^][()\" \t\n;']+\\)")
> +                  ;; Inside an expression of the form (SYM ARG1
> +                  ;; ARG2 ... ARGn) where SYM has a `lisp-indent-function'
> +                  ;; property that's a number, insert a newline after
> +                  ;; the corresponding ARGi, because it tends to lead to
> +                  ;; more natural and less indented code.
> +                  (let* ((sym (intern-soft (match-string 1)))
> +                         (lif (and sym (get sym 'lisp-indent-function))))
> +                    (if (eq lif 'defun) (setq lif 2))
> +                    (when (natnump lif)
> +                      (goto-char (match-end 0))
> +                      (forward-sexp lif)

I only want to give the hint that this `forward-sexp' in the last line
will error when the programmer has, e.g., commented out the body of a
lambda expression.  Or the user wants to fill automatically created code
that looks like that, for whatever reason.

Michael.

Re: master 2f181d60323 3/6: pp.el (pp-fill): New default pp function

[Stefan Monnier]

> I only want to give the hint that this `forward-sexp' in the last line
> will error when the programmer has, e.g., commented out the body of a
> lambda expression.  Or the user wants to fill automatically created code
> that looks like that, for whatever reason.

Hmm... I don't quite understand what you mean...
Oh, are you thinking of when `lif` is a number larger than the number of
elements in the list?
Indeed, I think we'll burp then.  I'll take a look at that.


        Stefan

Re: master 2f181d60323 3/6: pp.el (pp-fill): New default pp function

[Michael Heerdegen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> > I only want to give the hint that this `forward-sexp' in the last line
> > will error when the programmer has, e.g., commented out the body of a
> > lambda expression.  Or the user wants to fill automatically created code
> > that looks like that, for whatever reason.
>
> Hmm... I don't quite understand what you mean...
> Oh, are you thinking of when `lif` is a number larger than the number of
> elements in the list?

Yes.

> Indeed, I think we'll burp then.  I'll take a look at that.

Thanks.  I would vote for silently ignoring and falling back to default
indentation.


BTW, we had a thread in the Gnus mailing list
(info-gnus-english@gnu.org) about `pp-fill'.  Now that it's the default
pp behavior, Gnus score files will be filled using this function before
saving.  I don't use score files, could be using this function is not
appropriate for this kind of data.

Anyway, a user reported a massive slowdown after you had made that
change (thread "slow saving of scores when leaving a virtual (nnml+)
group"), and posted this profiler output:

| Cpu report (partly expanded):
| 
|        10133  79% - command-execute
|         8519  66%  - funcall-interactively
|         4767  37%   - gnus-summary-exit
|         4659  36%    - gnus-score-save
|         4655  36%     - gnus-pp
|         4655  36%      - pp
|         4655  36%       - pp-to-string
|         4655  36%        - pp-fill
|         4647  36%         - pp--object
|         4627  36%          - pp-fill
|         4615  36%           - pp-fill
|         4555  35%            - pp-fill
|         4263  33%             - pp-fill
|         4243  33%              - indent-according-to-mode
|         4243  33%               - lisp-indent-line
|         4163  32%                - calculate-lisp-indent
|         4163  32%                 - lisp-indent-function
|         4163  32%                    lisp--local-defform-body-p
|           48   0%                + lisp-ppss
|          240   1%             + indent-according-to-mode
|           72   0%    + gnus-run-hooks
|           32   0%    + gnus-close-group
|            4   0%    + gnus-summary-update-info
|         3688  28%   + gnus-topic-select-group
|           60   0%   + file-notify-handle-event
|            3   0%   + execute-extended-command
|         1614  12%  + byte-code
|         2260  17%   Automatic GC
|          407   3% + timer-event-handler
|            0   0%   ...

Dunno if it is interesting for you.  So far I don't know how the data
looked like, but we can request it from the user.  After changing
`pp-default-function' to 'pp-28' the user is happy.


Michael.

Re: master 2f181d60323 3/6: pp.el (pp-fill): New default pp function

[Stefan Monnier]

>> Indeed, I think we'll burp then.  I'll take a look at that.
> Thanks.  I would vote for silently ignoring and falling back to default
> indentation.

Indeed.

> BTW, we had a thread in the Gnus mailing list
> (info-gnus-english@gnu.org) about `pp-fill'.  Now that it's the default
> pp behavior, Gnus score files will be filled using this function before
> saving.  I don't use score files, could be using this function is not
> appropriate for this kind of data.

I'm always ambivalent about the use of `pp` for "internal data files"
like these.  I suspect they'd often be better served with a plain
`prin1`, but I'd be interesting to hear the motivation behind the use
of `pp`.

> Anyway, a user reported a massive slowdown after you had made that
> change (thread "slow saving of scores when leaving a virtual (nnml+)
> group"), and posted this profiler output:
>
> | Cpu report (partly expanded):
> | 
> |        10133  79% - command-execute
> |         8519  66%  - funcall-interactively
> |         4767  37%   - gnus-summary-exit
> |         4659  36%    - gnus-score-save
> |         4655  36%     - gnus-pp
> |         4655  36%      - pp
> |         4655  36%       - pp-to-string
> |         4655  36%        - pp-fill
> |         4647  36%         - pp--object
> |         4627  36%          - pp-fill
> |         4615  36%           - pp-fill
> |         4555  35%            - pp-fill
> |         4263  33%             - pp-fill
> |         4243  33%              - indent-according-to-mode
> |         4243  33%               - lisp-indent-line
> |         4163  32%                - calculate-lisp-indent
> |         4163  32%                 - lisp-indent-function
> |         4163  32%                    lisp--local-defform-body-p
> |           48   0%                + lisp-ppss
> |          240   1%             + indent-according-to-mode
> |           72   0%    + gnus-run-hooks
> |           32   0%    + gnus-close-group
> |            4   0%    + gnus-summary-update-info
> |         3688  28%   + gnus-topic-select-group
> |           60   0%   + file-notify-handle-event
> |            3   0%   + execute-extended-command
> |         1614  12%  + byte-code
> |         2260  17%   Automatic GC
> |          407   3% + timer-event-handler
> |            0   0%   ...

Hmm... that looks bad, indeed, and looks (again) like a performance
problem in `lisp-indent-line`.

More specifically the `lisp--local-defform-body-p` up there suggests the
problem was introduced by the special indentation rules for `cl-flet`.

> Dunno if it is interesting for you.  So far I don't know how the data
> looked like, but we can request it from the user.

That would be nice, yes.  Opening an Emacs bug report for it would make
a lot of sense as well.

> After changing `pp-default-function' to 'pp-28' the user is happy.

If Gnus doesn't want to use `prin1` than maybe it should use `pp-28` or
even some other/new alternative designed for "fast pp of data": we
should be able to go significantly faster than `pp-28`.  E.g. we could
forgo indentation altogether and only insert \n at appropriate places,
which would not only be fast but result is smaller files.


        Stefan

Re: master 2f181d60323 3/6: pp.el (pp-fill): New default pp function

[Eric S Fraga]

On Saturday, 15 Jul 2023 at 11:19, Stefan Monnier wrote:
>> Dunno if it is interesting for you.  So far I don't know how the data
>> looked like, but we can request it from the user.
>
> That would be nice, yes.  Opening an Emacs bug report for it would make
> a lot of sense as well.

The user was me; I will try to open a bug report later today. But I'm
here should you want to know anything further etc.

-- 
Eric S Fraga via gnus (Emacs 30.0.50 2023-07-10) on Debian 12.0

Re: master 2f181d60323 3/6: pp.el (pp-fill): New default pp function

[Eric S Fraga]

Emacs bug reported: bug#64681
Thank you,
eric
-- 
Eric S Fraga via gnus (Emacs 30.0.50 2023-07-10) on Debian 12.0