Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Po Lu]

Gregory Heytings <gregory@heytings.org> writes:

> +	       doc: /* Line length above which specific display optimizations are used.
> +Display optimizations for long lines will automatically be enabled in
> +buffers which contain one or more lines whose length is above that
> +threshold.
> +When nil, these display optimizations are disabled.  */);

What are the "specific display optimizations"?

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Gregory Heytings <gregory@heytings.org>
> Date: Thu, 21 Jul 2022 20:28:52 +0800
> 
> Gregory Heytings <gregory@heytings.org> writes:
> 
> > +	       doc: /* Line length above which specific display optimizations are used.
> > +Display optimizations for long lines will automatically be enabled in
> > +buffers which contain one or more lines whose length is above that
> > +threshold.
> > +When nil, these display optimizations are disabled.  */);
> 
> What are the "specific display optimizations"?

Read the code ;-)

Or maybe I don't understand what you are asking about.

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Gregory Heytings]

>> +	       doc: /* Line length above which specific display optimizations are used.
>> +Display optimizations for long lines will automatically be enabled in
>> +buffers which contain one or more lines whose length is above that
>> +threshold.
>> +When nil, these display optimizations are disabled.  */);
>
> What are the "specific display optimizations"?
>

I don't understand your question.  These optimizations are hidden in the 
bowels of the display engine.  If you're interested in the details, you 
can read the code.

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Read the code ;-)

I think it would be more useful to explain what the user can (and can
not) expect from those "specific optimizations".  Otherwise, it is not
possible to make an intelligent decision about the value of that option.

For example, describing that vertical-motion is still slow when point is
on a long line would be very helpful.

If that isn't possible, then it would be good to at least explain that
the optimization (note the singular case) is temporary narrowing of the
buffer during redisplay.

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Gregory Heytings]

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


>
> I think it would be more useful to explain what the user can (and can 
> not) expect from those "specific optimizations".  Otherwise, it is not 
> possible to make an intelligent decision about the value of that option.
>

One of the design principles of that feature is that it should be as 
transparent as possible, and it is.  The point of adding a variable to 
control the feature was to avoid introducing an arbitrary constant at the 
C level, and also to allow users who really enjoy slowdowns to turn these 
optimizations off 😉  Its default value should be fine for everyone, there 
is no "intelligent decision" to make here.

>
> For example, describing that vertical-motion is still slow when point is 
> on a long line would be very helpful.
>

Vertical-motion is not slow anymore.

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  gregory@heytings.org
> Date: Thu, 21 Jul 2022 21:13:26 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Read the code ;-)
> 
> I think it would be more useful to explain what the user can (and can
> not) expect from those "specific optimizations".  Otherwise, it is not
> possible to make an intelligent decision about the value of that option.
> 
> For example, describing that vertical-motion is still slow when point is
> on a long line would be very helpful.
> 
> If that isn't possible, then it would be good to at least explain that
> the optimization (note the singular case) is temporary narrowing of the
> buffer during redisplay.

It's WIP.  It's too early to describe it in such detail, because we
don't yet know the details ourselves.  And even if we knew, the doc
string is probably not a good place for those technicalities.  A doc
string for a variable such as this one should speak in terms
understandable by users who aren't redisplay experts.

When we know exactly what this does, we should revisit this doc string
and see if anything should be done about it.  Meanwhile, I made small
adjustments in the wording.

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Po Lu]

Gregory Heytings <gregory@heytings.org> writes:

> One of the design principles of that feature is that it should be as
> transparent as possible, and it is.  The point of adding a variable to
> control the feature was to avoid introducing an arbitrary constant at
> the C level, and also to allow users who really enjoy slowdowns to
> turn these optimizations off

Then why is it a number and not a boolean? A number that I don't know
how to change is as good as a constant.

> Vertical-motion is not slow anymore.

Could you please show a file where I can appreciate a noticable speed-up
in vertical-motion?

Thanks.

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Gregory Heytings]

>> One of the design principles of that feature is that it should be as 
>> transparent as possible, and it is.  The point of adding a variable to 
>> control the feature was to avoid introducing an arbitrary constant at 
>> the C level, and also to allow users who really enjoy slowdowns to turn 
>> these optimizations off
>
> Then why is it a number and not a boolean? A number that I don't know 
> how to change is as good as a constant.
>

You don't have to change it.

>> Vertical-motion is not slow anymore.
>
> Could you please show a file where I can appreciate a noticable speed-up 
> in vertical-motion?
>

Are you kidding?  Try, for example:

https://github.com/Wilfred/ReVo-utilities/blob/a4bdc40dd2656c496defc461fc19c403c8306d9f/revo-export/dictionary.json?raw=true

No type M-> C-p.  Before the merge this takes about 50 seconds (about 20 
seconds for M-> and 30 seconds for C-p).  Now it is instantaneous.  And 
that's only a moderately large file (20 MB).  Try the same with a 1 GB 
file, and M-> will never end.

Meanwhile, could you please fix the build breakage introduced by 
a29a3ad55d?

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Thu, 21 Jul 2022 21:26:50 +0800
> 
> Gregory Heytings <gregory@heytings.org> writes:
> 
> > One of the design principles of that feature is that it should be as
> > transparent as possible, and it is.  The point of adding a variable to
> > control the feature was to avoid introducing an arbitrary constant at
> > the C level, and also to allow users who really enjoy slowdowns to
> > turn these optimizations off
> 
> Then why is it a number and not a boolean? A number that I don't know
> how to change is as good as a constant.

If we decide that there's no reason for users to change the number, we
might consider making it a boolean, indeed.

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Gregory Heytings]

>> Then why is it a number and not a boolean? A number that I don't know 
>> how to change is as good as a constant.
>
> If we decide that there's no reason for users to change the number, we 
> might consider making it a boolean, indeed.
>

Not a boolean, because even if there is no reason for users to change the 
number, there are at least three useful values: always disabled, enabled 
iff the buffer contains long lines, always enabled.  Which you can get now 
by setting the variable to nil, 10000, and 0.

I'm also pretty sure we would have bug reports if what a "long line" is 
was hardcoded.  So from my point of view the variable is fine as it is.

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Po Lu]

Gregory Heytings <gregory@heytings.org> writes:

> You don't have to change it.

So why make it an option and document it as such?  What is the purpose?

> Are you kidding?  Try, for example:
>
> https://github.com/Wilfred/ReVo-utilities/blob/a4bdc40dd2656c496defc461fc19c403c8306d9f/revo-export/dictionary.json?raw=true
>
> No type M-> C-p.  Before the merge this takes about 50 seconds (about
> 20 seconds for M-> and 30 seconds for C-p).  Now it is instantaneous.
> And that's only a moderately large file (20 MB).  Try the same with a
> 1 GB file, and M-> will never end.

Right.  But a change from 50 seconds to 20 seconds is not enough for
pixel-scroll-precision-mode.

I guess redisplay is now faster, but vertical-movement itself is still
as slow as it was.

> Meanwhile, could you please fix the build breakage introduced by
> a29a3ad55d?

What build breakage?  a29a3ad55d worked on all platforms when I tested
it, so please show the build error.

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Po Lu]

Gregory Heytings <gregory@heytings.org> writes:

> Not a boolean, because even if there is no reason for users to change
> the number, there are at least three useful values: always disabled,
> enabled iff the buffer contains long lines, always enabled.  Which you
> can get now by setting the variable to nil, 10000, and 0.

Then why isn't that written in the doc string?  And with more obvious
values: nil, t, always.

> I'm also pretty sure we would have bug reports if what a "long line"
> is was hardcoded.  So from my point of view the variable is fine as it
> is.

Right, so users will complain about a constant in a supposedly
transparent feature?  Then it's not as transparent as you think it is.

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Gregory Heytings]

>> No type M-> C-p.  Before the merge this takes about 50 seconds (about 
>> 20 seconds for M-> and 30 seconds for C-p).  Now it is instantaneous. 
>> And that's only a moderately large file (20 MB).  Try the same with a 1 
>> GB file, and M-> will never end.
>
> Right.  But a change from 50 seconds to 20 seconds is not enough for 
> pixel-scroll-precision-mode.
>

Please read the sentence above again.  "Before the merge this takes about 
50 seconds.  Now it is intantaneous."  Is that not enough for you to 
"appreciate a noticable speed-up"?

>> Meanwhile, could you please fix the build breakage introduced by 
>> a29a3ad55d?
>
> What build breakage?  a29a3ad55d worked on all platforms when I tested 
> it, so please show the build error.
>

I sent you the error a few hours ago:

cedet/semantic/symref/list.el:35:2: Error: Wrong type argument: number-or-marker-p, nil

(with make bootstrap).

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Po Lu]

Gregory Heytings <gregory@heytings.org> writes:

> Please read the sentence above again.  "Before the merge this takes
> about 50 seconds.  Now it is intantaneous."  Is that not enough for
> you to "appreciate a noticable speed-up"?

I see, but it was not instantaneous, so I didn't understand your
sentence very well.

> I sent you the error a few hours ago:
>
> cedet/semantic/symref/list.el:35:2: Error: Wrong type argument: number-or-marker-p, nil
>
> (with make bootstrap).

That is certainly unrelated to a29a3ad55d.  (And I didn't receive any
email, possibly due to the spam filter.)

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Gregory Heytings]

>> I'm also pretty sure we would have bug reports if what a "long line" is 
>> was hardcoded.  So from my point of view the variable is fine as it is.
>
> Right, so users will complain about a constant in a supposedly 
> transparent feature?  Then it's not as transparent as you think it is.
>

I won't continue this discussion, sorry.

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Gregory Heytings]

>> I sent you the error a few hours ago:
>>
>> cedet/semantic/symref/list.el:35:2: Error: Wrong type argument: number-or-marker-p, nil
>>
>> (with make bootstrap).
>
> That is certainly unrelated to a29a3ad55d.  (And I didn't receive any 
> email, possibly due to the spam filter.)
>

It is not unrelated, that's the result of a git bisect with make 
bootstrap.  (This was a post on emacs-devel.)

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Eli Zaretskii]

> Date: Thu, 21 Jul 2022 13:50:41 +0000
> From: Gregory Heytings <gregory@heytings.org>
> cc: Po Lu <luangruo@yahoo.com>, emacs-devel@gnu.org
> 
> 
> >> Then why is it a number and not a boolean? A number that I don't know 
> >> how to change is as good as a constant.
> >
> > If we decide that there's no reason for users to change the number, we 
> > might consider making it a boolean, indeed.
> >
> 
> Not a boolean, because even if there is no reason for users to change the 
> number, there are at least three useful values: always disabled, enabled 
> iff the buffer contains long lines, always enabled.  Which you can get now 
> by setting the variable to nil, 10000, and 0.

A tristate is also a possibility.

> I'm also pretty sure we would have bug reports if what a "long line" is 
> was hardcoded.  So from my point of view the variable is fine as it is.

I didn't say it wasn't, just that we might revisit this when the we
have more feedback and data points.

Re: master 616da8fa8e: Merge branch 'feature/fix-the-long-lines-display-bug'

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Thu, 21 Jul 2022 22:02:37 +0800
> 
> Gregory Heytings <gregory@heytings.org> writes:
> 
> > Please read the sentence above again.  "Before the merge this takes
> > about 50 seconds.  Now it is intantaneous."  Is that not enough for
> > you to "appreciate a noticable speed-up"?
> 
> I see, but it was not instantaneous, so I didn't understand your
> sentence very well.

Is your build optimized or unoptimized?  In an unoptimized build it is
indeed not instantaneous, but it's faster.

> > I sent you the error a few hours ago:
> >
> > cedet/semantic/symref/list.el:35:2: Error: Wrong type argument: number-or-marker-p, nil
> >
> > (with make bootstrap).
> 
> That is certainly unrelated to a29a3ad55d.

Actually, it is related, and should be fixed now.