Re: [Emacs-diffs] master de76a16: Performance improvements for vc-hg

Re: [Emacs-diffs] master de76a16: Performance improvements for vc-hg

[Stefan Monnier]

>>>>> "Daniel" == Daniel Colascione <dan.colascione@gmail.com> writes:
>     Teach vc-hg how to read some Mercurial internal data structures,
>     allowing us to avoid the need to run hg status -A, which is very slow
>     for large repositories.  Fall back to running hg if anything looks
>     funny.  vc-hg now puts the _working directory_ revision in the
>     modeline instead of the file revision, which greatly improves
>     performance and which allows us to again skip running hg in the case
>     that we have an active bookmark.

Thanks Daniel.  Having written the same kind of thing for Bzr (tho it
was much simpler in that case), and taking a step back, I think this
reflects a mistake in VC: we probably just shouldn't provide such
detailed info in the modeline, or at least not until the user makes
explicit use of VC.

Having to go through the trouble of re-implementing part of the VCS to
parse the internal data to figure out the working revision, even though
we have no idea if the user will ever care, is deeply unsatisfactory.


        Stefan

Re: [Emacs-diffs] master de76a16: Performance improvements for vc-hg

[Daniel Colascione]

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

On 02/08/2016 11:55 AM, Stefan Monnier wrote:
>>>>>> "Daniel" == Daniel Colascione <dan.colascione@gmail.com> writes:

Huh. Where is that old email address coming from?

>>     Teach vc-hg how to read some Mercurial internal data structures,
>>     allowing us to avoid the need to run hg status -A, which is very slow
>>     for large repositories.  Fall back to running hg if anything looks
>>     funny.  vc-hg now puts the _working directory_ revision in the
>>     modeline instead of the file revision, which greatly improves
>>     performance and which allows us to again skip running hg in the case
>>     that we have an active bookmark.
> 
> Thanks Daniel.  Having written the same kind of thing for Bzr (tho it
> was much simpler in that case), and taking a step back, I think this
> reflects a mistake in VC: we probably just shouldn't provide such
> detailed info in the modeline, or at least not until the user makes
> explicit use of VC.
> 
> Having to go through the trouble of re-implementing part of the VCS to
> parse the internal data to figure out the working revision, even though
> we have no idea if the user will ever care, is deeply unsatisfactory.

I find the information useful, and we can now get it cheaply. Users can
easily remove vc-hg from vc-handled-backends if they don't want the
functionality.

What might be worthwhile is making it easier to use vc for a file if
that file's correct vc backend isn't vc-handled-backends --- that is, in
that mode, we'd initialize the vc goo only when the user asks for it. I
don't think that mode should be the default though.


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

Re: [Emacs-diffs] master de76a16: Performance improvements for vc-hg

[Eli Zaretskii]

> From: Daniel Colascione <dancol@dancol.org>
> Date: Mon, 8 Feb 2016 12:11:46 -0800
> 
> > Having to go through the trouble of re-implementing part of the VCS to
> > parse the internal data to figure out the working revision, even though
> > we have no idea if the user will ever care, is deeply unsatisfactory.
> 
> I find the information useful, and we can now get it cheaply.

We are paying a non-trivial price as a project for that information,
see for example bug#21559.  Maybe it should be an option, off by
default?

> Users can easily remove vc-hg from vc-handled-backends if they don't
> want the functionality.

That's hardly user-friendly, IMO.

Re: [Emacs-diffs] master de76a16: Performance improvements for vc-hg

[Daniel Colascione]

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

On 02/08/2016 12:21 PM, Eli Zaretskii wrote:
>> From: Daniel Colascione <dancol@dancol.org>
>> Date: Mon, 8 Feb 2016 12:11:46 -0800
>>
>>> Having to go through the trouble of re-implementing part of the VCS to
>>> parse the internal data to figure out the working revision, even though
>>> we have no idea if the user will ever care, is deeply unsatisfactory.
>>
>> I find the information useful, and we can now get it cheaply.
> 
> We are paying a non-trivial price as a project for that information,
> see for example bug#21559.  Maybe it should be an option, off by
> default?

bug#21559 is a corner case.

vc has been in place for decades, and is one of the things that makes
programming on Emacs pleasant. I'm not in favor of just removing the
feature because there are a few unfixed bugs. There is nothing
fundamentally wrong with the model.

Making vc itself an option that's off by default makes no sense. Under
what circumstances should a user enable the feature? "When he's willing
to pay the cost" is the answer I expect. How is the user supposed to
know what the cost is? How is he supposed to know the feature even
exists? Every feature has a cost; in the case of vc, that cost is well
worth the benefit.

There is no reason that vc integration can't work well and be on by
default. It was working before my change; now it works better. A
reasonable performance improvement should not be an occasion for
discussing the removal of a feature made faster.


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

Re: [Emacs-diffs] master de76a16: Performance improvements for vc-hg

[Eli Zaretskii]

> Cc: monnier@IRO.UMontreal.CA, emacs-devel@gnu.org
> From: Daniel Colascione <dancol@dancol.org>
> Date: Mon, 8 Feb 2016 12:28:29 -0800
> 
> >> I find the information useful, and we can now get it cheaply.
> > 
> > We are paying a non-trivial price as a project for that information,
> > see for example bug#21559.  Maybe it should be an option, off by
> > default?
> 
> bug#21559 is a corner case.

It could be the tip of an iceberg.

> vc has been in place for decades, and is one of the things that makes
> programming on Emacs pleasant. I'm not in favor of just removing the
> feature because there are a few unfixed bugs. There is nothing
> fundamentally wrong with the model.
> 
> Making vc itself an option that's off by default makes no sense.

I didn't suggest making VC an option.  I suggested to make running the
"status" command optional.

> Under what circumstances should a user enable the feature?

When she wants the mode line to display whatever it is that we display
there.

"When
> he's willing to pay the cost" is the answer I expect.


> How is he supposed to know the feature even exists?

How do users know about the existence of any other feature?  This one
is no different.

> There is no reason that vc integration can't work well and be on by
> default. It was working before my change; now it works better.

For a single VCS, and not the most important one.

The problem with calling "status" is that locks the repository, makes
changes in the filesystem, and interferes with Emacs features that are
sensitive to changes in the filesystem.  Making it optional could be
the only solution to that conundrum.

> A reasonable performance improvement should not be an occasion for
> discussing the removal of a feature made faster.

No one suggested removing the feature, this is a misunderstanding.

Re: [Emacs-diffs] master de76a16: Performance improvements for vc-hg

[Daniel Colascione]

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

On 02/08/2016 01:01 PM, Eli Zaretskii wrote:
>> Cc: monnier@IRO.UMontreal.CA, emacs-devel@gnu.org
>> From: Daniel Colascione <dancol@dancol.org>
>> Date: Mon, 8 Feb 2016 12:28:29 -0800
>>
>>>> I find the information useful, and we can now get it cheaply.
>>>
>>> We are paying a non-trivial price as a project for that information,
>>> see for example bug#21559.  Maybe it should be an option, off by
>>> default?
>>
>> bug#21559 is a corner case.
> 
> It could be the tip of an iceberg.

If it were, I'd have expected problems to crop up for a long time. vc
has been running status on find-file for a very long time.

>> vc has been in place for decades, and is one of the things that makes
>> programming on Emacs pleasant. I'm not in favor of just removing the
>> feature because there are a few unfixed bugs. There is nothing
>> fundamentally wrong with the model.
>>
>> Making vc itself an option that's off by default makes no sense.
> 
> I didn't suggest making VC an option.  I suggested to make running the
> "status" command optional.
> 
>> Under what circumstances should a user enable the feature?
> 
> When she wants the mode line to display whatever it is that we display
> there.
> 
> "When
>> he's willing to pay the cost" is the answer I expect.
> 
> 
>> How is he supposed to know the feature even exists?
> 
> How do users know about the existence of any other feature?  This one
> is no different.

You could use the same argument to suggest making auto-mode-alist blank
by default. Users can enable the modes they want, and we wouldn't want
to impose costs on users by enabling features they don't want.

It's okay for some features to be on by default. vc is one of those
features. I have seen nothing to suggest that we can't keep doing what
we've been doing for a very long time.

> 
>> There is no reason that vc integration can't work well and be on by
>> default. It was working before my change; now it works better.
> 
> For a single VCS, and not the most important one.
> 
> The problem with calling "status" is that locks the repository, makes
> changes in the filesystem, and interferes with Emacs features that are
> sensitive to changes in the filesystem.  Making it optional could be
> the only solution to that conundrum.

It's a classical lock ordering problem; the classical lock ordering
solutions apply, including using try-lock instead of locking
repositories and running the status command asynchronously, so that if
it never completes, interactivity isn't harmed.

Not doing anything that might "make[] changes in the filsystem" is far
too cautious.



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

VC's modeline (was: [Emacs-diffs] master de76a16: Performance improvements for vc-hg)

[Stefan Monnier]

> vc has been in place for decades, and is one of the things that makes
> programming on Emacs pleasant. I'm not in favor of just removing the
> feature because there are a few unfixed bugs. There is nothing
> fundamentally wrong with the model.

Agreed.

What I think is wrong the level of detail expected in the status line.
E.g. I think it'd be fine for VC to just display "Bzr" (or "Hg", or
"Git", or what have you) instead.  I think it'd even be fine to do that
without checking whether the file is really under that VCS's control or
not (i.e. without checking whether the file is ignored).  IOW, a file
opened in a directory with a ".git" somewhere in the parents could just
say "Git", without any other check.

> There is no reason that vc integration can't work well and be on by
> default.

Having to call a "git" process twice per find-file-noselect is a high
price for a very small benefit.

> It was working before my change; now it works better.

Yes, I'm really not arguing against your change.

> A reasonable performance improvement should not be an occasion for
> discussing the removal of a feature made faster.

Sorry.  It just reminded me of this problem that's been around for a while.


        Stefan

Re: VC's modeline

[Óscar Fuentes]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> What I think is wrong the level of detail expected in the status line.
> E.g. I think it'd be fine for VC to just display "Bzr" (or "Hg", or
> "Git", or what have you) instead.  I think it'd even be fine to do that
> without checking whether the file is really under that VCS's control or
> not (i.e. without checking whether the file is ignored).  IOW, a file
> opened in a directory with a ".git" somewhere in the parents could just
> say "Git", without any other check.

Saying that the file is on a directory that also has a .git (or .bzr, or
whatever) directory is almost meaningless. You don't know if the file is
actually versioned, nor if it is edited, nor the branch that is
currently checked out (this last piece of information is specially
important on workflows that uses colocated branches.)

Personally, I find having that information on the modeline a very worthy
investment. In practice, the only problem I experience is when working
on MS Windows, where calling git is much slower than on GNU/Linux and it
is annoying to wait until all buffers refreshes the VC info after some
operation with Magit.

If you wish to add a customization for disabling the modeline VC info
(all or part) I know that some users will appreciate the feature. But
reducing the information to a meaningless label is not a sane default.

Re: VC's modeline

[Daniel Colascione]

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



On 02/08/2016 01:39 PM, Stefan Monnier wrote:
>> vc has been in place for decades, and is one of the things that makes
>> programming on Emacs pleasant. I'm not in favor of just removing the
>> feature because there are a few unfixed bugs. There is nothing
>> fundamentally wrong with the model.
> 
> Agreed.
> 
> What I think is wrong the level of detail expected in the status line.

I'd _like_ to see arbitrarily large amounts of detail on the modeline
provided that we can display this information safely and efficiently.
Why not? If one VCS in particular can't provide this information in a
timely manner, sure, we can override vc-mode-line-string and display
something more spartan. But the ideal should be to show as much as possible.

> E.g. I think it'd be fine for VC to just display "Bzr" (or "Hg", or
> "Git", or what have you) instead.  I think it'd even be fine to do that
> without checking whether the file is really under that VCS's control or
> not (i.e. without checking whether the file is ignored).  IOW, a file
> opened in a directory with a ".git" somewhere in the parents could just
> say "Git", without any other check.
> 
>> There is no reason that vc integration can't work well and be on by
>> default.
> 
> Having to call a "git" process twice per find-file-noselect is a high
> price for a very small benefit.
> 
>> It was working before my change; now it works better.
> 
> Yes, I'm really not arguing against your change.
> 
>> A reasonable performance improvement should not be an occasion for
>> discussing the removal of a feature made faster.
> 
> Sorry.  It just reminded me of this problem that's been around for a while.

Yes, I think that thread got off on the wrong foot. It's better now.


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

Re: VC's modeline

[Stefan Monnier]

>> What I think is wrong the level of detail expected in the status line.
>> E.g. I think it'd be fine for VC to just display "Bzr" (or "Hg", or
>> "Git", or what have you) instead.  I think it'd even be fine to do that
>> without checking whether the file is really under that VCS's control or
>> not (i.e. without checking whether the file is ignored).  IOW, a file
>> opened in a directory with a ".git" somewhere in the parents could just
>> say "Git", without any other check.
> Saying that the file is on a directory that also has a .git (or .bzr, or
> whatever) directory is almost meaningless.

It indicates which backend would be used if the user wants to perform
a VC operation.  E.g. for non-managed files, it'd indicate which backend
will be used for `C-x v i' (vc-register).

> You don't know if the file is actually versioned, nor if it is edited,

In my experience 99% of Emacs users have no idea that the ":" between
the backend name and the "version info" means that the file is
locally modified.

Of the remaining 1%, some (at least me; and I presume I'm not the only
one) never look at it and just use `C-x v =' to see what's changed.

So this info is not of very high value.

> nor the branch that is currently checked out (this last piece of
> information is specially important on workflows that uses
> colocated branches.)

That's occasionally useful in Git, indeed.  Luckily it's easy/cheap to
obtain this info in Git.

> Personally, I find having that information on the modeline a very worthy
> investment.  In practice, the only problem I experience is when working
> on MS Windows, where calling git is much slower than on GNU/Linux and it
> is annoying to wait until all buffers refreshes the VC info after some
> operation with Magit.

See, we do have a problem.  I'm not opposed to having refined info
available, but the current setup where this refined info is
"indispensable" and always needs to be computed eagerly is problematic.
We should arrange for those things to be computed more lazily (and
maybe for that we need to change the UI so it's also *displayed* more
lazily, e.g. only after the user does something).


        Stefan

Re: VC's modeline

[Óscar Fuentes]

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

>> Saying that the file is on a directory that also has a .git (or .bzr, or
>> whatever) directory is almost meaningless.
>
> It indicates which backend would be used if the user wants to perform
> a VC operation.  E.g. for non-managed files, it'd indicate which backend
> will be used for `C-x v i' (vc-register).

Why should you care about the backend? If you wish to register a file,
you do it regardless of the backend.

A user can easily lost track of the state of a buffer (is it edited?
does it corresponds with the contents of the file on disk? to which
branch does in belong?) But if the user forgets about which VCS he is
using (suppossing that that info is relevant to him) he has more serious
problems than Emacs being slow :-)

>> You don't know if the file is actually versioned, nor if it is edited,
>
> In my experience 99% of Emacs users have no idea that the ":" between
> the backend name and the "version info" means that the file is
> locally modified.
>
> Of the remaining 1%, some (at least me; and I presume I'm not the only
> one) never look at it and just use `C-x v =' to see what's changed.

The info that the VC state gives you is that something changed. Using
C-x v = for just checking if the file contains changes is quite a lot of
work.

> So this info is not of very high value.

It is of high value for me. And indeed the colon is hard to notice.
That's why I added faces to each of those VC states.

>> nor the branch that is currently checked out (this last piece of
>> information is specially important on workflows that uses
>> colocated branches.)
>
> That's occasionally useful in Git, indeed.  Luckily it's easy/cheap to
> obtain this info in Git.

Executing a shell command or a M-x command (if there exists one for that
purpose) is never as easy as seeing the info right there when you switch
to the buffer. It also shows that you are not going to edit some stale
buffer that is out of sync with its file after a git operation (this
also applies to the VC file state.)

>> Personally, I find having that information on the modeline a very worthy
>> investment.  In practice, the only problem I experience is when working
>> on MS Windows, where calling git is much slower than on GNU/Linux and it
>> is annoying to wait until all buffers refreshes the VC info after some
>> operation with Magit.
>
> See, we do have a problem.

It is a problem for some users. For me and many others it is just an
annoyance on certain specific circunstances.

> I'm not opposed to having refined info
> available, but the current setup where this refined info is
> "indispensable" and always needs to be computed eagerly is problematic.
> We should arrange for those things to be computed more lazily (and
> maybe for that we need to change the UI so it's also *displayed* more
> lazily, e.g. only after the user does something).

I agree wholeheartedly here. But please note that there is no "eager
computation" of the VC modeline, AFAIK. It happens by explicit request
from packages like Magit or when the user has auto-revert-mode enabled
on those buffers.

Re: VC's modeline

[Stefan Monnier]

> Why should you care about the backend?

E.g. because there may be various valid options (see vc-switch-backend).

> A user can easily lose track of the state of a buffer (is it edited?
> does it corresponds with the contents of the file on disk? to which
> branch does in belong?) But if the user forgets about which VCS he is
> using (suppossing that that info is relevant to him) he has more serious
> problems than Emacs being slow :-)

Could be.  Basically, the "Git" (or "Bzr" ...) would mostly mean "Hey,
VC noticed that there's a VCS in use around here".  So, yes we could
replace it with a red pulsating dot which means that VC is "activated".
But to decide whether to put the red pulsating dot we end up necessarily
finding out which backend is in use, so we may as well display the
backend's name instead.  It also has the advantage that the user will
probably instinctively know that this "Git" thingy means that Emacs
found something out in relation to a VCS, whereas we'd have to explain
what the red pulsating dot means (same thing if we use "VC" since the
user may not necessarily know that Emacs's builtin generic VCS support
is called "VC").

> The info that the VC state gives you is that something changed.  Using
> C-x v = for just checking if the file contains changes is quite a lot of
> work.

I just described what I do.  I rarely (never?) only care if the file is
modified or not.  If it is modified I pretty much always want to see
what is changed.  So I never look at the modeline and just always use
`C-x v =' instead.

>> So this info is not of very high value.
> It is of high value for me. And indeed the colon is hard to notice.
> That's why I added faces to each of those VC states.

As mentioned, I do think it's good that this info can be displayed.
What I think is wrong is to compute this info eagerly/unconditionally by
default.
E.g. we compute this info even if the file is never displayed.
Of course this gets worse if find-file-noselect is called in a loop to
collect info about various files none of which are ever displayed.

> It is a problem for some users. For me and many others it is just an
> annoyance on certain specific circunstances.

Indeed.  It's also a problem that we have to write ad-hoc code to try
and parse internal Bzr/Hg/... data to try and avoid those performance
problems, which in turn implies more maintenance.  Reducing the amount
of detail in the VC modeline would also be a problem for some users.

So we should try and find some solution that avoids *all* those problems.

> I agree wholeheartedly here. But please note that there is no "eager
> computation" of the VC modeline, AFAIK.

I think there is, in vc-find-file-hook.


        Stefan

Re: VC's modeline

[Markus Triska]

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

> In my experience 99% of Emacs users have no idea that the ":" between
> the backend name and the "version info" means that the file is
> locally modified.

I agree! I think using "*" would be a much better indicator to indicate
that the file is modified locally, in analogy to the "**" that is used
to show that the Emacs buffer is modified.

> So this info is not of very high value.

At least to me, this info *is* of very high value! I think it only
should be made more readable, using "*".

All the best,
Markus

Re: VC's modeline

[Markus Triska]

Daniel Colascione <dancol@dancol.org> writes:

> I'd _like_ to see arbitrarily large amounts of detail on the modeline
> provided that we can display this information safely and efficiently.
> Why not? If one VCS in particular can't provide this information in a
> timely manner, sure, we can override vc-mode-line-string and display
> something more spartan. But the ideal should be to show as much as
> possible.

I agree with this 100%.

All the best,
Markus

Re: VC's modeline

[Dmitry Gutov]

On 02/10/2016 05:16 PM, Stefan Monnier wrote:

> As mentioned, I do think it's good that this info can be displayed.
> What I think is wrong is to compute this info eagerly/unconditionally by
> default.
> E.g. we compute this info even if the file is never displayed.

I second this. But along these lines, we should compute the current 
backend lazily as well.

Until the buffer's mode line is being displayed (i.e. the buffer is 
visible), or we call some VC command, there's no need to know the 
buffer's backend.

 > Of course this gets worse if find-file-noselect is called in a loop to
 > collect info about various files none of which are ever displayed.

Right, this would also make it easier to implement find-file-delayed 
(the facility to visit buffers in the background quickly, in a read-only 
fashion, concealed from most user-level commands).