Permission requested to merge branch comment-cache into master.

Permission requested to merge branch comment-cache into master.

[Alan Mackenzie]

Hello, John.

Could I merge branch comment-cache into master, please?

Background:

in bug #22884, Paul reported a simple edit in config.h taking around 10
seconds to echo.  The cause was an open parenthesis in column zero which
Emacs's low level routines (in syntax.c) wrongly parsed as a start of
defun.

I have reformulated and grossly simplified the routines which scan
comments, now scanning them exclusively in the forward direction (which
is easy) and recording them (and strings) with text properties.  The old
routines attempt (and sometimes fail) to scan comments going backwards.
Parentheses in column zero have entirely ceased to be an issue - there
is no longer even code to handle them.  (OK, the old code is still there
- I haven't removed it yet.)

The new scheme adds a small time overhead (~0.25s to scan xdisp.c on my
6 y.o. machine), and will also use a modest amount of extra storage for
the text properties.  After the initial scan, movement over comments is
lightening fast.

I am confident that the new approach is right, and I am confident the
new code is reliable and ready for prime time, even if, perhaps, not yet
entirely bug-free.

Alternatives:

An alternative approach using the syntax-ppss cache has been discussed
and tried.  For reasons currently being discussed in bug #22983:
(syntax-ppss returns wrong result) and the emacs-devel thread "Problems
with syntax-ppss: Was [... Apply `comment-depth' text properties when
calling `back_comment'.]", this approach cannot work.  It also does
nothing to tame the complexity of the current comment scanning routines.

So, I'd like to merge branch comment-cache, at least on a trial basis.
What do you say?

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Permission requested to merge branch comment-cache into master.

[Eli Zaretskii]

> Date: Sat, 12 Mar 2016 00:28:37 +0000
> From: Alan Mackenzie <acm@muc.de>
> Cc: emacs-devel@gnu.org
> 
> Could I merge branch comment-cache into master, please?

Why master?  Why not emacs-25?  The slow performance of CC mode is an
annoying misfeature, IMO, which we should strive to fix in Emacs 25.1.

Re: Permission requested to merge branch comment-cache into master.

[Alan Mackenzie]

Good morning, Eli.

On Sat, Mar 12, 2016 at 09:32:19AM +0200, Eli Zaretskii wrote:
> > Date: Sat, 12 Mar 2016 00:28:37 +0000
> > From: Alan Mackenzie <acm@muc.de>
> > Cc: emacs-devel@gnu.org

> > Could I merge branch comment-cache into master, please?

> Why master?  Why not emacs-25?

The change is a fairly radical new approach, and there could still be
bugs in the new code, or unforeseen problems.  But in principle, I have
nothing against merging it into emacs-25.

> The slow performance of CC mode is an annoying misfeature, IMO, which
> we should strive to fix in Emacs 25.1.

I honestly don't think this change is going to speed up CC Mode very
much at all.  A little bit, perhaps, but not very much.  For example, in
Martin's `foo' and `bar' (defuns which do `beginning/end-of-defun'
repeatedly) traversing xdisp.c, there was a saving of 6 - 8 seconds in
each direction, but the real time sink was checking for K&R
declarations in the backward direction.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Permission requested to merge branch comment-cache into master.

[Eli Zaretskii]

> Date: Sat, 12 Mar 2016 10:08:43 +0000
> Cc: johnw@gnu.org, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > The slow performance of CC mode is an annoying misfeature, IMO, which
> > we should strive to fix in Emacs 25.1.
> 
> I honestly don't think this change is going to speed up CC Mode very
> much at all.

I don't understand: the original complaint for config.h was about an
unbearably slow response to a simple editing command.  Are you saying
that these changes will not speed up that use case?

Re: Permission requested to merge branch comment-cache into master.

[Alan Mackenzie]

Hello, Eli.

On Sat, Mar 12, 2016 at 01:14:34PM +0200, Eli Zaretskii wrote:
> > Date: Sat, 12 Mar 2016 10:08:43 +0000
> > Cc: johnw@gnu.org, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > > The slow performance of CC mode is an annoying misfeature, IMO, which
> > > we should strive to fix in Emacs 25.1.

> > I honestly don't think this change is going to speed up CC Mode very
> > much at all.

> I don't understand: the original complaint for config.h was about an
> unbearably slow response to a simple editing command.  Are you saying
> that these changes will not speed up that use case?

Sorry, yes, in that particular case the change will speed it up
dramatically.  That case is (i) When there is a paren in column zero in
a comment; (ii) There is a lot of syntactic whitespace and no "normal"
code between that paren and the spot being edited.  I see this more as a
bug fix than a performance enhancement.

But for CC Mode in general, I don't see a dramatic speed up happening
because of this change.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Permission requested to merge branch comment-cache into master.

[Eli Zaretskii]

> Date: Sat, 12 Mar 2016 11:50:21 +0000
> Cc: johnw@gnu.org, emacs-devel@gnu.org
> From: Alan Mackenzie <acm@muc.de>
> 
> > > > The slow performance of CC mode is an annoying misfeature, IMO, which
> > > > we should strive to fix in Emacs 25.1.
> 
> > > I honestly don't think this change is going to speed up CC Mode very
> > > much at all.
> 
> > I don't understand: the original complaint for config.h was about an
> > unbearably slow response to a simple editing command.  Are you saying
> > that these changes will not speed up that use case?
> 
> Sorry, yes, in that particular case the change will speed it up
> dramatically.

Then I think this should go to the release branch.  If there's a way
to test it better on a branch, fine.  Otherwise, let's include it in
the next pretest, and let the crowd find any problems it could still
have.

Thanks.

Re: Permission requested to merge branch comment-cache into master.

[Alan Mackenzie]

Hello, Eli.

On Sat, Mar 12, 2016 at 02:31:55PM +0200, Eli Zaretskii wrote:
> > Date: Sat, 12 Mar 2016 11:50:21 +0000
> > Cc: johnw@gnu.org, emacs-devel@gnu.org
> > From: Alan Mackenzie <acm@muc.de>

> > Sorry, yes, in that particular case the change will speed it up
> > dramatically.

> Then I think this should go to the release branch.  If there's a way
> to test it better on a branch, fine.  Otherwise, let's include it in
> the next pretest, and let the crowd find any problems it could still
> have.

OK, let's wait to see what John says.  In the meantime, I'll need to
write a little documentation on the minor restrictions that will come
with the code.  These are things like binding
`inhibit-modification-hooks' to t when changing any text properties to
avoid emptying the cache unnecessarily, and suchlike.

> Thanks.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Permission requested to merge branch comment-cache into master.

[John Wiegley]

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

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

> Then I think this should go to the release branch. If there's a way to test
> it better on a branch, fine. Otherwise, let's include it in the next
> pretest, and let the crowd find any problems it could still have.

This change may be ready for 25.2 at a future date, but I don't believe it's
ready now.

Also, there is still enough debate around this feature from others, that I
prefer it to incubate on a feature branch at this time. Especially, I'd like
to see if there's a way to integrate your efforts with Stefan's, rather than
developing two separate systems to address similar ideas.

Thanks,
-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 629 bytes --]

Re: Permission requested to merge branch comment-cache into master.

[Alan Mackenzie]

Hello, John.

On Sat, Mar 12, 2016 at 01:38:54PM -0800, John Wiegley wrote:
> >>>>> Eli Zaretskii <eliz@gnu.org> writes:

> > Then I think this should go to the release branch. If there's a way to test
> > it better on a branch, fine. Otherwise, let's include it in the next
> > pretest, and let the crowd find any problems it could still have.

> This change may be ready for 25.2 at a future date, but I don't believe it's
> ready now.

I would say it's not quite ready, but it's not far off.

> Also, there is still enough debate around this feature from others, that I
> prefer it to incubate on a feature branch at this time.

That's where it is, on branch comment-cache.  But it would be nicer to
hear "actively test" than "incubate".  ;-)

> Especially, I'd like to see if there's a way to integrate your efforts
> with Stefan's, rather than developing two separate systems to address
> similar ideas.

This is not possible, and the suggestion that it is is mischievous.  The
two approaches are fundamentally different.  They aim to do different
things, and they do them in very different ways.

The comment-cache code is designed optimally to handle comments and
strings and those things only, and to handle them at lightening speed.
It is also written with simplicity and maintainability in mind.
`syntax-ppss' is more general, handling general parse states, and does
so with less speed.  There is absolutely no reason why these two systems
cannot exist at the same time.  Indeed, they should.

`syntax-ppss' is broken (see bug #22983): it does not do what its
specification says, and what its specification says is unsuitable for
use in the low level syntax routines.  It is possible a similar function
may be developed to take the place of `syntax-ppss' sometime in the
future, but this will need development and testing, and around 200 calls
to it in the Emacs sources will need checking and possibly adaptating.

By contrast, the code in the comment-cache branch is ready now and,
modulo minor bugs, is working.  There has been no intimation from
anybody that it doesn't work.  It would be a shame for it to languish
unused on a feature branch.

You mentioned in another post that you were concerned about the speed of
CC Mode.  Well, I have prepared an emacs-25 build and a comment-cache
build, strictly comparable, and timed Martin Rudalics's two functions
`foo' and `bar' on xdisp.c.  `foo' repeatedly calls `end-of-defun',
`bar' repeatedly calls `beginning-of-defun'.  Here are the timings:

             emacs-25            comment-cache       increase in speed
foo            2.590                 1.662                56%
bar            2.981                 1.628                83%

Thus on this (admittedly limited) test, comment-cache has delivered a
massive speed boost to CC Mode, something I hadn't expected.  There is
more speed to be wrung out of the comment-cache mechanism - where now
there is much scanning forward from "safe positions" (i.e. positions not
in comments and strings), in comment-cache this could be rendered
unnecessary, since the nearest safe position is no further away than a
couple of text property lookups.

By contrast, `syntax-ppss' works by keeping a list of positions 20,000
characters apart, and scanning forward from these position to produce
the parse state at the desired position.  This cannot compete speedwise
with comment-cache.

Where, then, do we go from here?  Just how important is CC Mode's lack
of performance?

> Thanks,
> -- 
> John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
> http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Permission requested to merge branch comment-cache into master.

[John Wiegley]

>>>>> Alan Mackenzie <acm@muc.de> writes:

> This is not possible, and the suggestion that it is is mischievous. 

I try to reserve my mischief for things are much more fun, I assure you. :)

> The two approaches are fundamentally different. They aim to do different
> things, and they do them in very different ways.

What I don't want to happen is that a disagreement between contributors
results in the maintenance of two separate solutions to similar problems, just
because of a difference in design and a lack of agreement.

Stefan had the prerogative to include his solution before, so I understand why
it's in emacs-25 now. But your concerns lead me to believe that his code
should be revisited. If `syntax-ppss' is not doing the job we need it to do --
to the extent that you have to implement new code to work around it -- that
indicate a potential problem.

I also want to make sure it's not just a documentation failure, or lack of
communication, before either ditching syntax-ppss, or introducing a new
mechanism to live alongside it.

Is it possible to summarize why syntax-ppss is not fulfilling your needs,
other than it being buggy? Because if that's the only issue, we can fix the
code.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Permission requested to merge branch comment-cache into master.

[Alan Mackenzie]

Hello, John.

On Sat, Mar 12, 2016 at 04:02:28PM -0800, John Wiegley wrote:
> >>>>> Alan Mackenzie <acm@muc.de> writes:

> > The two approaches are fundamentally different. They aim to do different
> > things, and they do them in very different ways.

> What I don't want to happen is that a disagreement between contributors
> results in the maintenance of two separate solutions to similar problems, just
> because of a difference in design and a lack of agreement.

Tell me, why is this concern being raised only now, after I've invested
a substantial amount of time and energy in fixing the problem?  I was
perfectly open about what I intended to do, the discussion taking place
in bug #22884 (where you were yourself active), and Paul and Eli were
supportive.  Surely that was the time to make representations about the
suitability of some alternative facility?

At this point I have to start asking myself how much I want to do in
Emacs.  If each time I consider tackling a big problem and openly
discuss this with other contributors, I face the prospect of my patches
being rejected for extrinsic reasons, I could perhaps well spend my time
doing other things instead.

> Stefan had the prerogative to include his solution before, so I understand why
> it's in emacs-25 now. But your concerns lead me to believe that his code
> should be revisited. If `syntax-ppss' is not doing the job we need it to do --
> to the extent that you have to implement new code to work around it -- that
> indicate a potential problem.

No, that is a misrepresentation.  I did not "work around" syntax-ppss.
My new code is not in any sense a "work around" - it is a carefully
crafted solution to the problem (see below).  syntax-ppss is simply
unsuitable for use in a primitive, and using it never entered into my
considerations.  It is difficult to understand, difficult to maintain
high level lisp and it calls out to user functions via a hook.  It is
crazy and in bad taste to make a primitive depend on such a thing.  It
is just the Wrong Thing to do.  Anything we use in primitives should be
robust and rigorous, and as far as possible, simple.

The basic problem I've solved is that of comments getting scanned in the
backward direction.  Comments _cannot_, in general, be scanned going
backwards.  The attempt is what gave rise to bug #22884, and quite a few
other bugs before it.  My solution is to scan comments going only
forwards.  This works, is robust, and doesn't require calling out to any
fragile lisp code.  Stefan's idea for a solution is to construe the
problem as something more limited, leave the backward scanning in place,
and just make bug #22884 go away (until the next bug happens).

> I also want to make sure it's not just a documentation failure, or lack of
> communication, before either ditching syntax-ppss, or introducing a new
> mechanism to live alongside it.

> Is it possible to summarize why syntax-ppss is not fulfilling your needs,
> other than it being buggy? Because if that's the only issue, we can fix the
> code.

syntax-ppss does the Wrong Thing.  It purports to return the equivalent
of (parse-partial-sexp (point-min) pos).  Should a buffer be narrowed
such that point-min is inside a string, syntax-ppss's view of the rest
of the buffer gets inverted: strings become non-strings and vice-versa.
For example, in the following syntactically correct C:

    char foo[] = "asdf asdf" "asdf"; /* "asdf" */ /*  */  /*   '"'"  */
                      ^

, narrow it such that point-min is at the marked point.  syntax-ppss
then sees the following strings, but no comments:

    " "
    "; /* "
    " */ /*  */  /*   '"
    "  */      ; no closing string quote

(forward-comment -1) from the end of line, using syntax-ppss, thus
fails.  In comment-cache, of course, it works without trouble.

What I predict is now going to happen is that a different function,
based on syntax-ppss, but returning the equivalent of
(parse-partial-sexp 1 pos) is going to get written.  I also predict that
it will be given the name syntax-ppss, and the 200 or so calls in our
code, and an indeterminate number in users' code are going to be left as
they are to fend for themselves, regardless of the change in
functionality.

How about, as a compromise, merging comment-cache into master for now,
while syntax-ppss gets sorted out, then deciding what to do once that
has happened?

> -- 
> John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
> http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Permission requested to merge branch comment-cache into master.

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 613 bytes --]

On 03/13/2016 11:20 AM, Alan Mackenzie wrote:
> What I predict is now going to happen is that a different function,
> based on syntax-ppss, but returning the equivalent of
> (parse-partial-sexp 1 pos) is going to get written.  I also predict that
> it will be given the name syntax-ppss, and the 200 or so calls in our
> code, and an indeterminate number in users' code are going to be left as
> they are to fend for themselves, regardless of the change in
> functionality.

I suggested that in an earlier message, but I don't think you responded :/
Do you think it would break anything?

Clément.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Permission requested to merge branch comment-cache into master.

[Dmitry Gutov]

On 03/13/2016 06:04 PM, Clément Pit--Claudel wrote:
> On 03/13/2016 11:20 AM, Alan Mackenzie wrote:
>> What I predict is now going to happen is that a different function,
>> based on syntax-ppss, but returning the equivalent of
>> (parse-partial-sexp 1 pos) is going to get written.  I also predict that
>> it will be given the name syntax-ppss, and the 200 or so calls in our
>> code, and an indeterminate number in users' code are going to be left as
>> they are to fend for themselves, regardless of the change in
>> functionality.
>
> I suggested that in an earlier message, but I don't think you responded :/
> Do you think it would break anything?

There's not much point in creating a different function (or else it 
would have to come with its own set of global variables, and hence will 
require to duplicated some more code as well). Anyway, this is 
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=22983, where the ball is in 
Alan's court.

So far, it looks trivial to fix, code-wise.

Re: Permission requested to merge branch comment-cache into master.

[Alan Mackenzie]

Hello, Clément.

On Sun, Mar 13, 2016 at 12:04:04PM -0400, Clément Pit--Claudel wrote:
> On 03/13/2016 11:20 AM, Alan Mackenzie wrote:
> > What I predict is now going to happen is that a different function,
> > based on syntax-ppss, but returning the equivalent of
> > (parse-partial-sexp 1 pos) is going to get written.  I also predict that
> > it will be given the name syntax-ppss, and the 200 or so calls in our
> > code, and an indeterminate number in users' code are going to be left as
> > they are to fend for themselves, regardless of the change in
> > functionality.

> I suggested that in an earlier message, ....

You did indeed.

> ..... but I don't think you responded :/
> Do you think it would break anything?

Yes, I do.  With ~200 calls to the function, some of them are going to
depend on it doing precisely what the specification says.  Possibly not
very many, but some will.

> Clément.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Permission requested to merge branch comment-cache into master.

[Stefan Monnier]

> Yes, I do.  With ~200 calls to the function, some of them are going to
> depend on it doing precisely what the specification says.  Possibly not
> very many, but some will.

I wouldn't worry about that: unless they pay extra care to flush the
cache manually, they currently sometimes get one behavior
sometimes another, so they're already broken.


        Stefan

Re: Permission requested to merge branch comment-cache into master.

[Alan Mackenzie]

Hello, Stefan.

On Sun, Mar 13, 2016 at 06:27:58PM -0400, Stefan Monnier wrote:
> > Yes, I do.  With ~200 calls to the function, some of them are going to
> > depend on it doing precisely what the specification says.  Possibly not
> > very many, but some will.

> I wouldn't worry about that: unless they pay extra care to flush the
> cache manually, they currently sometimes get one behavior
> sometimes another, so they're already broken.

They're not broken, not of themselves.  Would you agree that because
syntax-ppss doesn't always return the defined value according to its
spec, we should move away from using it?

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Permission requested to merge branch comment-cache into master.

[Clément Pit--Claudel]

[-- Attachment #1.1: Type: text/plain, Size: 861 bytes --]

On 03/13/2016 06:52 PM, Alan Mackenzie wrote:
> They're not broken, not of themselves.  Would you agree that because
> syntax-ppss doesn't always return the defined value according to its
> spec, we should move away from using it?

Hi Alan,

Before I read this thread, I never really paid attention to the fact that syntax-ppss needed widening; I would guess (but of course I can't be sure) that many did the same thing. In fact, I've only ever used syntax-ppss to decide whether I was in a comment, or in a string, and possibly where it sarted, so if syntax-ppss was changed my code would benefit.
On the other hand, if a better behaved alternative is introduced, my code will have to be manually updated; thus I would very much like it if syntax-ppss was changed :)

Just offering an opinion of course. Thanks for your efforts on this!
Clément.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Permission requested to merge branch comment-cache into master.

[Stefan Monnier]

> They're not broken, not of themselves.  Would you agree that because
> syntax-ppss doesn't always return the defined value according to its
> spec, we should move away from using it?

No, otherwise I would have "moved away from using" Emacs a long time ago.
Instead, my reaction to such situations is to try and *fix* them ;-)

So, go ahead, make the change, see where it breaks, and fix those cases.


        Stefan

Re: Permission requested to merge branch comment-cache into master.

[Andreas Röhler]

On 13.03.2016 23:52, Alan Mackenzie wrote:
> Hello, Stefan.
>
> On Sun, Mar 13, 2016 at 06:27:58PM -0400, Stefan Monnier wrote:
>>> Yes, I do.  With ~200 calls to the function, some of them are going to
>>> depend on it doing precisely what the specification says.  Possibly not
>>> very many, but some will.
>> I wouldn't worry about that: unless they pay extra care to flush the
>> cache manually, they currently sometimes get one behavior
>> sometimes another, so they're already broken.
> They're not broken, not of themselves.  Would you agree that because
> syntax-ppss doesn't always return the defined value according to its
> spec, we should move away from using it?
>
>

Hi Alan,

that's why I purged my code of syntax-ppss. Ironically, having another 
look at during these recent discussions, the interest is back. What 
about using the difference, taking profit of two different states in 
narrowed vs. widened buffer?

Re: Permission requested to merge branch comment-cache into master.

[Alan Mackenzie]

Hello, Andreas.

On Mon, Mar 14, 2016 at 07:32:39AM +0100, Andreas Röhler wrote:


> On 13.03.2016 23:52, Alan Mackenzie wrote:

> Hi Alan,

> Ironically, having another look at during these recent discussions,
> the interest is back. What about using the difference, taking profit
> of two different states in narrowed vs. widened buffer?

There is no way to "subtract" two parse-partial-sexp states, except,
possibly, in special circumstances.  Consider, these two states may both
be inside a comment or string, in which case parse-partial-sexp won't
record any additional info inside of that string.  But if you started at
pos-1 (inside the string) and parsed forward to pos-2 (still inside the
string) you might well record parentheses, and so on.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Permission requested to merge branch comment-cache into master.

[John Wiegley]

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

>>>>> Alan Mackenzie <acm@muc.de> writes:

> Hello, John.

Hi Alan,

I won't have the time for a proper response until a bit later, but I wanted to
say: please don't become discouraged. I'm very glad you're paying attention to
these issues. In the end, I think I want exactly what you want, the only
question is the path we take to get there.

I'll consider everything you wrote very carefully this week, but until then, I
wanted you to know my gratitude for all the efforts you've made so far.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 629 bytes --]

Re: Permission requested to merge branch comment-cache into master.

[Alan Mackenzie]

Hello, John.

On Sun, Mar 13, 2016 at 10:07:37AM -0700, John Wiegley wrote:
> Hi Alan,

> I won't have the time for a proper response until a bit later, but I wanted to
> say: please don't become discouraged. I'm very glad you're paying attention to
> these issues. In the end, I think I want exactly what you want, the only
> question is the path we take to get there.

> I'll consider everything you wrote very carefully this week, but until then, I
> wanted you to know my gratitude for all the efforts you've made so far.

Thanks, that's appreciated.

> -- 
> John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
> http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Permission requested to merge branch comment-cache into master.

[Dmitry Gutov]

On 03/13/2016 05:20 PM, Alan Mackenzie wrote:

(Again, not John.)

> Tell me, why is this concern being raised only now, after I've invested
> a substantial amount of time and energy in fixing the problem?  I was
> perfectly open about what I intended to do, the discussion taking place
> in bug #22884 (where you were yourself active), and Paul and Eli were
> supportive.

Please don't be surprised that, since the discussion of the new 
generic-ish feature happened inside a relatively specialized bug report, 
not everyone who could provide input, did so there.

Re: Permission requested to merge branch comment-cache into master.

[Stefan Monnier]

> syntax-ppss is simply unsuitable for use in a primitive, and using it
> never entered into my considerations.  It is difficult to understand,
> difficult to maintain high level lisp

Huh?  syntax-ppss is very simple code, and I have no idea where you
could have gotten the idea that it's "difficult to maintain".

> and it calls out to user functions via a hook.  It is crazy and in bad
> taste to make a primitive depend on such a thing.  It is just the
> Wrong Thing to do.

The only case where it calls to user function is via syntax-propertize.
In the case of back_comment this should never happen (because we're
moving backward, so syntax-propertize has already been called on
positions further down and hence isn't needed at this position), but in
case it were to happen it could only be because it's necessary *for
correctness*.

So, again: when my patch to back_comment calls syntax-ppss, it will not
call syntax-propertize-function.  Of course, it still means we call an
Elisp function, which could be adviced and whatnot.  If you want to
rewrite it in C go for it, but I'd oppose it unless you have good
*concrete* reasons for it, rather than vague allegations about it being
"crazy" or "wrong".

> Anything we use in primitives should be robust and rigorous, and as
> far as possible, simple.

Exactly.  But in my book, robust includes "has the needed syntax-table
properties applied".

> Stefan's idea for a solution is to construe the problem as something
> more limited, leave the backward scanning in place, and just make bug
> #22884 go away (until the next bug happens).

Alan, you're a good coder and good thinker.  I know you can make
a much better case against my code than that kind of FUD.

I have acknowledged all the weaknesses you point out (hell, I could give
you a few more if you were more welcoming).  But syntax-ppss was
introduced in Emacs-22.1, i.e. it has close to 9 years of wide use.
So these issues aren't nearly as serious as you make it out to be.
And furthermore, if/when they need to be fixed, they can be probably be
fixed rather easily.

>     char foo[] = "asdf asdf" "asdf"; /* "asdf" */ /*  */  /*   '"'"  */
>                       ^
> , narrow it such that point-min is at the marked point.  syntax-ppss
> then sees the following strings, but no comments:

As mentioned, depending on the *reason* behind narrowing, what you say
that syntax-ppss does actually makes perfect sense.  Better yet: that's
also what (forward-comment -1) does in Emacs-24.  And it's not because
of a bug in (forward-comment -1) but because (forward-comment -1) also
decided to start parsing from (point-min) in that particular case.

Now, I'm far from opposed to changing the behavior so as to treat the
last /*   '"'"  */ as a comment in the narrowed region.  I think this
choice doesn't really matter much, except for a few specific
circumstances, where the caller needs to provide more info in order to
clarify which behavior she wants.

[ Of course, syntax-ppss will sometimes do one and sometimes the other,
  depending on the state of its cache.  So, yes, syntax-ppss's behavior in
  this respect is inconsistent.  And noone has reported a single bug about
  it in all those years.
  Should we fix this problem?  Why not!
  Is it urgent?  Imperative?  Worthy of writing a whole new system
  (which fundamentally suffers from the exact same problem just with
  different arbitrary choices)?  Really?  ]

>     " "
>     "; /* "
>     " */ /*  */  /*   '"
>     "  */      ; no closing string quote
>
> (forward-comment -1) from the end of line, using syntax-ppss, thus
> fails.

And as mentioned it also fails in all versions of Emacs released so far.

> In comment-cache, of course, it works without trouble.

Hmm... sure sounds like a backward incompatibility to me.

> What I predict is now going to happen is that a different function,
> based on syntax-ppss, but returning the equivalent of
> (parse-partial-sexp 1 pos) is going to get written.  I also predict that
> it will be given the name syntax-ppss, and the 200 or so calls in our
> code, and an indeterminate number in users' code are going to be left as
> they are to fend for themselves, regardless of the change in
> functionality.

Right, because they are already fending for themselves against
syntax-ppss's inconsistent behavior in the presence of narrowing.

> How about, as a compromise, merging comment-cache into master for now,
> while syntax-ppss gets sorted out, then deciding what to do once that
> has happened?

You make it sound like your solution is rock-solid bullet-proof whereas
syntax-ppss is completely wobbly.  This verges on the ridiculous.
They're both wobbly, by the very nature of their work.


        Stefan

Re: Permission requested to merge branch comment-cache into master.

[Alan Mackenzie]

Hello, Stefan.

On Sun, Mar 13, 2016 at 09:11:04PM -0400, Stefan Monnier wrote:
> > syntax-ppss is simply unsuitable for use in a primitive, and using it
> > never entered into my considerations.  It is difficult to understand,
> > difficult to maintain high level lisp

> Huh?  syntax-ppss is very simple code, and I have no idea where you
> could have gotten the idea that it's "difficult to maintain".

OK, I take that back.  I personally found it difficult to understand,
and would not want to have to maintain it.

> > and it calls out to user functions via a hook.  It is crazy and in bad
> > taste to make a primitive depend on such a thing.  It is just the
> > Wrong Thing to do.

> The only case where it calls to user function is via syntax-propertize.
> In the case of back_comment this should never happen (because we're
> moving backward, so syntax-propertize has already been called on
> positions further down and hence isn't needed at this position), ....

That's not true.  If you have just moved forward in the buffer to a part
where syntax-properties haven't yet been applied, and do a
(forward-comment -1), syntax-propertize would get called for that buffer
position.

> but in case it were to happen it could only be because it's necessary
> *for correctness*.

It might be necessary to apply those properties for correctness, but
applying them is NOT the job of a primitive.

> So, again: when my patch to back_comment calls syntax-ppss, it will not
> call syntax-propertize-function.

It will.

> Of course, it still means we call an Elisp function, which could be
> adviced and whatnot.  If you want to rewrite it in C go for it, but
> I'd oppose it unless you have good *concrete* reasons for it, rather
> than vague allegations about it being "crazy" or "wrong".

I have an idea.  How about rewriting the cache mechanism in C, caching
the value MUCH more frequently (say, every 128 bytes) and using a data
structure suitable for that amount of data?

> > Anything we use in primitives should be robust and rigorous, and as
> > far as possible, simple.

> Exactly.  But in my book, robust includes "has the needed syntax-table
> properties applied".

That's the job of high level code, most likely part of a major mode.
The primitive should _respect_ the syntax-table properties which are
there, but has no business applying them itself.  Indeed the way they
are applied and when they are applied is entirely for the major mode to
decide.

> > Stefan's idea for a solution is to construe the problem as something
> > more limited, leave the backward scanning in place, and just make bug
> > #22884 go away (until the next bug happens).

> Alan, you're a good coder and good thinker.  I know you can make
> a much better case against my code than that kind of FUD.

OK, sorry about the invective, but take that away and what I said is
accurate.  You have construed the problem as much more limited in scope
than I have, and you would leave back_comment largely unchanged.

> I have acknowledged all the weaknesses you point out (hell, I could give
> you a few more if you were more welcoming).  But syntax-ppss was
> introduced in Emacs-22.1, i.e. it has close to 9 years of wide use.
> So these issues aren't nearly as serious as you make it out to be.
> And furthermore, if/when they need to be fixed, they can be probably be
> fixed rather easily.

> >     char foo[] = "asdf asdf" "asdf"; /* "asdf" */ /*  */  /*   '"'"  */
> >                       ^
> > , narrow it such that point-min is at the marked point.  syntax-ppss
> > then sees the following strings, but no comments:

> As mentioned, depending on the *reason* behind narrowing, what you say
> that syntax-ppss does actually makes perfect sense.  Better yet: that's
> also what (forward-comment -1) does in Emacs-24.  And it's not because
> of a bug in (forward-comment -1) but because (forward-comment -1) also
> decided to start parsing from (point-min) in that particular case.

Let's fix it.

> Now, I'm far from opposed to changing the behavior so as to treat the
> last /*   '"'"  */ as a comment in the narrowed region.  I think this
> choice doesn't really matter much, except for a few specific
> circumstances, where the caller needs to provide more info in order to
> clarify which behavior she wants.

Here's where we differ.  I think primitives should be rigorous and
always work, even in awkward corner cases.

[ .... ]

> > How about, as a compromise, merging comment-cache into master for now,
> > while syntax-ppss gets sorted out, then deciding what to do once that
> > has happened?

> You make it sound like your solution is rock-solid bullet-proof whereas
> syntax-ppss is completely wobbly.  This verges on the ridiculous.
> They're both wobbly, by the very nature of their work.

And that is wholly uncalled for.  My solution is rock-solid and
bullet-proof, barring any remaining bugs in it due to fact that it's new
code.  If you see any bugs in it, please let me (and everybody else)
know.  I would like to get the code merged into master specifically to
exercise it and find those bugs, have people measure performance, and so
on.  As for syntax-ppss, its problems are currently under discussion
elsewhere.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Permission requested to merge branch comment-cache into master.

[Stefan Monnier]

>> The only case where it calls to user function is via syntax-propertize.
>> In the case of back_comment this should never happen (because we're
>> moving backward, so syntax-propertize has already been called on
>> positions further down and hence isn't needed at this position), ....
> That's not true.  If you have just moved forward in the buffer to a part
> where syntax-properties haven't yet been applied, and do a
> (forward-comment -1), syntax-propertize would get called for that buffer
> position.

This should generally not be the case, no, because syntax-propertize
will have been called during the SETUP_SYNTAX_TABLE call earlier.
There might be corner cases where it is called, tho, admittedly.
Some of them may be bugs, obviously.

>> but in case it were to happen it could only be because it's necessary
>> *for correctness*.
> It might be necessary to apply those properties for correctness, but
> applying them is NOT the job of a primitive.

Depends if you want the primitive to work correctly.

>> So, again: when my patch to back_comment calls syntax-ppss, it will not
>> call syntax-propertize-function.
> It will.

The only case I know of where this can happen is if
parse-sexp-lookup-properties is nil (which is unusual since as soon as
syntax-propertize-function is called, it sets
parse-sexp-lookup-properties to t).  If needed we could try and plug
this corner case, tho it hasn't been a problem so far.

> I have an idea.  How about rewriting the cache mechanism in C, caching
> the value MUCH more frequently (say, every 128 bytes) and using a data
> structure suitable for that amount of data?

Be my guest.  It's always been part of the possible future I was
imagining for syntax-ppss, so I'm definitely not opposed to it.

That won't remove the need to call syntax-propertize, of course.
And I'm not sure it's worth the trouble since I haven't seen any
indication that the performance of the current implementation of
syntax-ppss is a problem.

> That's the job of high level code, most likely part of a major mode.
> The primitive should _respect_ the syntax-table properties which are
> there, but has no business applying them itself.  Indeed the way they
> are applied and when they are applied is entirely for the major mode to
> decide.

That's indeed what CC-mode does.  All other modes just use
syntax-propertize which takes care of that for them.  I have no reason
to believe that CC-mode couldn't also use syntax-propertize (tho it
would take a lot of work to restructure CC-mode accordingly, IIUC).

But it doesn't really matter: for various reasons, CC-mode does it
differently, and that's OK.  It means that in your case, syntax-ppss
will not call syntax-propertize-function, so the problems you're so
worried about won't affect you.

> OK, sorry about the invective, but take that away and what I said is
> accurate.  You have construed the problem as much more limited in scope
> than I have, and you would leave back_comment largely unchanged.

That's right, my patch leaves back_comment largely unchanged, and I do
think you're making mountains of mole-hills.

>> Now, I'm far from opposed to changing the behavior so as to treat the
>> last /*   '"'"  */ as a comment in the narrowed region.  I think this
>> choice doesn't really matter much, except for a few specific
>> circumstances, where the caller needs to provide more info in order to
>> clarify which behavior she wants.
> Here's where we differ.  I think primitives should be rigorous and
> always work, even in awkward corner cases.

Then you'd better start looking at the whole narrowing issue much
more seriously, because we don't handle it consistently at all:
syntax-ppss is just one of many culprits.

> And that is wholly uncalled for.  My solution is rock-solid and
> bullet-proof, barring any remaining bugs in it due to fact that it's new
> code.

Of course it's not.  According to your own description, it'll break when
syntax-table is changed; it will break when category properties are
changed; it will break when the buffer's text is changed while
inhibit-modification-hooks is non-nil.

Additionally to that it will break code which relies on
`forward-comment' (and all other primitives that depend on
parse-sexp-ignore-comments, such as scan-sexp, forward-sexp, up-list,
...) working *within* the narrowed region without widening.

I don't fault your code for that, mind you.  It's just that this is
Elisp we're talking about, not Coq.  The foundations on which you build
this have fuzzy&complex semantics.


        Stefan

Re: Permission requested to merge branch comment-cache into master.

[Eli Zaretskii]

> Date: Sat, 12 Mar 2016 23:17:13 +0000
> From: Alan Mackenzie <acm@muc.de>
> 
> You mentioned in another post that you were concerned about the speed of
> CC Mode.  Well, I have prepared an emacs-25 build and a comment-cache
> build, strictly comparable, and timed Martin Rudalics's two functions
> `foo' and `bar' on xdisp.c.  `foo' repeatedly calls `end-of-defun',
> `bar' repeatedly calls `beginning-of-defun'.  Here are the timings:
> 
>              emacs-25            comment-cache       increase in speed
> foo            2.590                 1.662                56%
> bar            2.981                 1.628                83%

My measurements (which I posted a few minutes ago) indicate that Emacs
25 is about twice slower than Emacs 23.3.  So the above means the
comment-cache branch brings the performance almost back to its Emacs
23 level.  Which is a Good Thing, I think.

> Where, then, do we go from here?  Just how important is CC Mode's lack
> of performance?

To me, it's very important, because I bump into the slow situations
way too much.

Re: Permission requested to merge branch comment-cache into master.

[Dmitry Gutov]

On 03/12/2016 02:28 AM, Alan Mackenzie wrote:

> Could I merge branch comment-cache into master, please?

I'm not John, but I wish we didn't hurry so much to merge experimental 
branches into master. The curly quotes feature comes to mind, and how it 
ended up very hard to back out of.

> An alternative approach using the syntax-ppss cache has been discussed
> and tried.  For reasons currently being discussed in bug #22983:
> (syntax-ppss returns wrong result) and the emacs-devel thread "Problems
> with syntax-ppss: Was [... Apply `comment-depth' text properties when
> calling `back_comment'.]", this approach cannot work.

This seems like a misrepresentation of the discussion in #22983. If 
anything, I'd expect the reverse conclusion: it's quite possible to make 
syntax-ppss stricter, and then base comments-cache on it.

Re: Permission requested to merge branch comment-cache into master.

[Alan Mackenzie]

Hello, Dmitry.

On Sat, Mar 12, 2016 at 03:23:45PM +0200, Dmitry Gutov wrote:
> On 03/12/2016 02:28 AM, Alan Mackenzie wrote:

> > Could I merge branch comment-cache into master, please?

> I'm not John, but I wish we didn't hurry so much to merge experimental 
> branches into master. The curly quotes feature comes to mind, and how it 
> ended up very hard to back out of.

This feature would be easier to remove: changes in 6 C files and some
(two or three) .texi files (not yet done).

> > An alternative approach using the syntax-ppss cache has been discussed
> > and tried.  For reasons currently being discussed in bug #22983:
> > (syntax-ppss returns wrong result) and the emacs-devel thread "Problems
> > with syntax-ppss: Was [... Apply `comment-depth' text properties when
> > calling `back_comment'.]", this approach cannot work.

> This seems like a misrepresentation of the discussion in #22983. If 
> anything, I'd expect the reverse conclusion: it's quite possible to make 
> syntax-ppss stricter, and then base comments-cache on it.

It would require a new function similar to syntax-ppss, but with a
different specification.  There are some other issues regarding
syntax-ppss being used in back_comment, too.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Permission requested to merge branch comment-cache into master.

[John Wiegley]

>>>>> Dmitry Gutov <dgutov@yandex.ru> writes:

> I'm not John, but I wish we didn't hurry so much to merge experimental
> branches into master. The curly quotes feature comes to mind, and how it
> ended up very hard to back out of.

I am very much in agreement with Dmitry on this point. Even on master, we
should show some reservation.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: Permission requested to merge branch comment-cache into master.

[Stefan Monnier]

> An alternative approach using the syntax-ppss cache has been discussed
> and tried.

It's a much smaller patch and no bug has been demonstrated with it
(other than artificial examples where it's not even unambiguous what
would be the right answer, because it's far from clear how we'd ever
get into such a situation).

In any case, I'm opposed to the way this is implemented, so if my voice has
any value, I want to record my strong opposition to it.

In case you don't want to dig into the whole thread, the core reason why
I'm opposed to it is because it introduces a parallel cache to the one
used in syntax-ppss, and I think we should only have a single cache
for that.

So if his patch is extended to completely replace syntax-ppss, my
objection would be addressed.


        Stefan