Re: emacs-27 b46c75b: xref-matches-in-files: Big Tramp speed-up

Re: emacs-27 b46c75b: xref-matches-in-files: Big Tramp speed-up

[Dmitry Gutov]

Hi Michael,

On 08.01.2020 11:03, Michael Albinus wrote:
> dgutov@yandex.ru (Dmitry Gutov) writes:
> 
>>      xref-matches-in-files: Big Tramp speed-up
>>
>>      * lisp/progmodes/xref.el (xref-matches-in-files):
>>      Greatly improve performance with remote files using Tramp
>>      (bug#34343).
> 
> Thanks. I've made a further improvement in tramp-file-local-name. In
> case NAME is not a Tramp file name, it calls file-local-name now.
> 
> This is to handle use cases like
> 
> (copy-file "/ssh:host:/path/index.html" "https://example.com/path/index.html")
> 
> and more subtle constellations.

Looks like a good change, functionality-wise. But speaking of the change 
below, I'll probably make it, but upon reading the code anybody would 
struggle to guess that this function can handle other local names, not 
just Tramp ones.

>> +      (setq files (mapcar
>> +                   (if (tramp-tramp-file-p dir)
>> +                       #'tramp-file-local-name
>> +                       #'file-local-name)
>> +                   files)))
> 
> You can change this now to
> 
> (setq files (mapcar #'tramp-file-local-name files))

This will shorten the code, and it'll require one fewer declare-function 
in the file.

But allow me to state for the record once more that I'm puzzled by the 
architectural choice we're working with here.

Re: emacs-27 b46c75b: xref-matches-in-files: Big Tramp speed-up

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Michael,

Hi Dmitry,

> Looks like a good change, functionality-wise. But speaking of the
> change below, I'll probably make it, but upon reading the code anybody
> would struggle to guess that this function can handle other local
> names, not just Tramp ones.

Yes. I made this change because I need it Tramp internal. No tramp-*
function is intended to be called outside Tramp itself except the ones
in the Tramp manual. By intention, I haven't documented it there, and I
have given it also a comment NOT to use it outside Tramp. You're the
only one so far I gave permission :-)

>>> +      (setq files (mapcar
>>> +                   (if (tramp-tramp-file-p dir)
>>> +                       #'tramp-file-local-name
>>> +                       #'file-local-name)
>>> +                   files)))
>> You can change this now to
>> (setq files (mapcar #'tramp-file-local-name files))
>
> This will shorten the code, and it'll require one fewer
> declare-function in the file.

And it avoids one tramp-tramp-file-p call.

> But allow me to state for the record once more that I'm puzzled by the
> architectural choice we're working with here.

I know that. Tramp follows the generic file name handler approach, as
described in (info "(elisp) Magic File Names") . This works fine for
single files, but might not be appropriate for filesets, as you happen
to be faced with. Maybe we need an architectural extension for a file
name handler of filesets. Or we add new magic operations, which work
over a fileset. Like vc operations do :-)

Best regards, Michael.

Re: emacs-27 b46c75b: xref-matches-in-files: Big Tramp speed-up

[Stefan Monnier]

>> But allow me to state for the record once more that I'm puzzled by the
>> architectural choice we're working with here.
> I know that.  Tramp follows the generic file name handler approach, as
> described in (info "(elisp) Magic File Names") .

Could someone explain to me why `file-local-name` has to be so much slower
than `tramp-file-local-name`?


        Stefan

Re: emacs-27 b46c75b: xref-matches-in-files: Big Tramp speed-up

[Michael Albinus]

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

>>> But allow me to state for the record once more that I'm puzzled by the
>>> architectural choice we're working with here.
>> I know that.  Tramp follows the generic file name handler approach, as
>> described in (info "(elisp) Magic File Names") .
>
> Could someone explain to me why `file-local-name` has to be so much slower
> than `tramp-file-local-name`?

Internally, `file-local-name' uses `file-remote-p'. The latter goes
through the file name handler machinery, and `tramp-file-name-handler'
eats time, according to Dmitry's measures. Usually not a problem, but
remarkable when you apply it to some ten thousand files as in
`xref-matches-in-files'.

>         Stefan

Best regards, Michael.

Re: emacs-27 b46c75b: xref-matches-in-files: Big Tramp speed-up

[Stefan Monnier]

>>>> But allow me to state for the record once more that I'm puzzled by the
>>>> architectural choice we're working with here.
>>> I know that.  Tramp follows the generic file name handler approach, as
>>> described in (info "(elisp) Magic File Names") .
>> Could someone explain to me why `file-local-name` has to be so much slower
>> than `tramp-file-local-name`?
> Internally, `file-local-name' uses `file-remote-p'. The latter goes
> through the file name handler machinery, and `tramp-file-name-handler'
> eats time, according to Dmitry's measures. Usually not a problem, but
> remarkable when you apply it to some ten thousand files as in
> `xref-matches-in-files'.

[ I think this should be in a comment in the code ;-)  ]
Hmm... I guess this deserves a bug number and we should try and figure
out why it's so much slower to go through the file-handler machinery
(and whether we can do something about it other than provide ad-hoc
coarse-grained operations).


        Stefan

Re: emacs-27 b46c75b: xref-matches-in-files: Big Tramp speed-up

[Michael Albinus]

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

> Hmm... I guess this deserves a bug number and we should try and figure
> out why it's so much slower to go through the file-handler machinery
> (and whether we can do something about it other than provide ad-hoc
> coarse-grained operations).

The file name handler machinery is designed for single files. If you
have some ten thousands files, which have all the same remote
identification, and for which the same operation will be applied, this
doesn't scale well. tramp-file-name-handler will be invoked for every
single file for the same operation, and whatever it does, it is lost
time. We would need the machinery only once.

That's why I have proposed to find a way to apply an operation to a
fileset instead of a single file only.

>         Stefan

Best regards, Michael.

Re: emacs-27 b46c75b: xref-matches-in-files: Big Tramp speed-up

[Stefan Monnier]

>> Hmm... I guess this deserves a bug number and we should try and figure
>> out why it's so much slower to go through the file-handler machinery
>> (and whether we can do something about it other than provide ad-hoc
>> coarse-grained operations).
> The file name handler machinery is designed for single files. If you
> have some ten thousands files, which have all the same remote
> identification, and for which the same operation will be applied, this
> doesn't scale well.

I understand that if we can treat a whole set of files which we know all
live below some directory, then some of the processing can be performed
just once instead of N times.

But in the current case, we just replace N calls to `file-local-name` with
N calls to `tramp-file-local-name` and it's supposedly already much
faster, even though `tramp-file-local-name` doesn't get to share work
between all those calls, AFAIK.  It's *this* speed difference which I'd
like to understand.

> That's why I have proposed to find a way to apply an operation to a
> fileset instead of a single file only.

Yes, the idea makes sense.  Basically add a magic-file-op
(file-list-apply FUNCTION DIR FILES &rest ARGS) where FILES are relative
to DIR?


        Stefan

Re: emacs-27 b46c75b: xref-matches-in-files: Big Tramp speed-up

[Michael Albinus]

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

> But in the current case, we just replace N calls to `file-local-name` with
> N calls to `tramp-file-local-name` and it's supposedly already much
> faster, even though `tramp-file-local-name` doesn't get to share work
> between all those calls, AFAIK.  It's *this* speed difference which I'd
> like to understand.

I said it already several times: it is tramp-file-name-handler, invoked
for every single file, which makes the major difference.

And I don't know how to change its work significantly.

>> That's why I have proposed to find a way to apply an operation to a
>> fileset instead of a single file only.
>
> Yes, the idea makes sense.  Basically add a magic-file-op
> (file-list-apply FUNCTION DIR FILES &rest ARGS) where FILES are relative
> to DIR?

Something like this, yes. However, in the case Dmitry is interested in,
FILES would be remote file names instead of relative file names. He
wanted to know the local file name part of the FILES.

I will see how it could look like. Later this week.

>         Stefan

Best regards, Michael.

Re: emacs-27 b46c75b: xref-matches-in-files: Big Tramp speed-up

[Stefan Monnier]

> I said it already several times: it is tramp-file-name-handler, invoked
> for every single file, which makes the major difference.

Yes, I saw that, but it's still not clear to me which part of this costs
so much more than tramp-file-local-name itself, nor why.

>> Yes, the idea makes sense.  Basically add a magic-file-op
>> (file-list-apply FUNCTION DIR FILES &rest ARGS) where FILES are relative
>> to DIR?
>
> Something like this, yes. However, in the case Dmitry is interested in,
> FILES would be remote file names instead of relative file names.

The "remote" pat would presumably be in the DIR argument (DIR would
presumably be the root of the project or something like that).

> He wanted to know the local file name part of the FILES.

Actually, maybe xref could apply `file-local-name` to the root of
the project?


        Stefan

Re: emacs-27 b46c75b: xref-matches-in-files: Big Tramp speed-up

[Dmitry Gutov]

On 08.01.2020 22:51, Stefan Monnier wrote:
> Yes, I saw that, but it's still not clear to me which part of this costs
> so much more than tramp-file-local-name itself, nor why.

The whole of it, mostly. I've shown a simple patch which improved 
performance a lot, by Michael says it's unmaintainable (IIUC):

https://debbugs.gnu.org/cgi/bugreport.cgi?bug=34343#83

(and there's a follow-up suggestion later)

By the way, the commit under discussion already references this bug report.

> The "remote" pat would presumably be in the DIR argument (DIR would
> presumably be the root of the project or something like that).

The current discussion is about xref-matches-in-files where Grep acts on 
a list of files, not a directory. It's used by both project-find-regexp 
and dired-do-find-regexp.

> Actually, maybe xref could apply `file-local-name` to the root of
> the project?

See above. It's not about projects only.

I've considered about having xref-matches-in-files also accept a 
(REMOTE-ID LOCAL-FILES) struct, but that's also increase in complexity, 
in all callers and related APIs, which will also affect maintainability. 
So it's doable, but I'd rather make a good effort and fix it in Tramp to 
have everyone benefit automatically.

Re: emacs-27 b46c75b: xref-matches-in-files: Big Tramp speed-up

[Philippe Vaucher]

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

>
> On 08.01.2020 22:51, Stefan Monnier wrote:
> > Yes, I saw that, but it's still not clear to me which part of this costs
> > so much more than tramp-file-local-name itself, nor why.
>
> The whole of it, mostly. I've shown a simple patch which improved
> performance a lot, by Michael says it's unmaintainable (IIUC):


I'm not sure wether what I'll say is relevant to this discussion (please
forgive me if it is not), but I just wanted to mention that a significant
part of "debugging TRAMP problems" in projects like projectile is related
to TRAMP's speed.

I always assumed it was due to the fact that checking file attributes over
SSH is unavoidably slower than doing it locally, but if TRAMP can be
improved for things like `locate-dominating-file` over TRAMP it'd be a huge
help.

Kind regards,
Philippe

[-- Attachment #2: Type: text/html, Size: 1138 bytes --]