bugGNU Octave - Bugs: bug #60387, ftp class method 'dir' extremely...

 
 

bug #60387: ftp class method 'dir' extremely slow

Submitter:  Muhali <muhali>
Submitted:  Tue 13 Apr 2021 12:44:53 PM UTC
   
 
Category:  Performance Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Performance
Status:  Confirmed Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 18 Apr 2021 12:34:51 AM UTC, comment #15: 

Attached is a patch which gets the roughly 2.5X speedup.  Unfortunately, Octave hangs when exiting.  I see no reason why this should be so, but something goes wrong.  Maybe jwe can see something simple.

(file #51285)

Rik <rik5>
Group administrator
Thu 15 Apr 2021 03:37:25 AM UTC, comment #14: 

The code is very similar to what we already do.

There was one difference, which was turning off this option


  curl_easy_setopt(curl, CURLOPT_HEADER, 0L);


I tried adding this to Octave's get_fileinfo() command and it didn't change timings.

What was substantive was removing this call at the end of get_fileinfo()


      // The MDTM command seems to reset the path to the root with the
      // servers I tested with, so cd again into the correct path.  Make
      // the path absolute so that this will work even with servers that
      // don't end up in the root after an MDTM command.
      //cwd ('/' + path);


This produced a 2.5X speedup, roughly what you were seeing in the difference from 14 sec (ftp) vs. 40 sec (Octave).

BUT, the file sizes after anything but the first entry are incorrect.  So, it seems that the cwd() command has to stay or even more debugging needs to be done.

Rik <rik5>
Group administrator
Wed 14 Apr 2021 05:57:43 PM UTC, comment #13: 

The attached example program (adapted from one I downloaded from the cURL web pages) appears to get accurate size and timestamp info without actually downloading the files.  It is faster than what we are doing in Octave but still slow.  I see about 14s for this program vs. 40 for Octave.  So probably the only good option for speed is to parse the output from a directory listing, but AFAIK, there is no uniform format for that output from an ftp server.

(file #51265)

John W. Eaton <jwe>
Group administrator
Wed 14 Apr 2021 12:15:49 AM UTC, comment #12: 

Good documentation can be found here: https://curl.se/libcurl/c/libcurl.html.

It looks to me like we are making other profound errors in our usage of libcurl.  In particular, it seems we should be calling curl_global_init() (https://curl.se/libcurl/c/curl_global_init.html) and curl_global_cleanup at end of program.  They are very emphatic that this call is not thread-safe and should be made before any other threads are created, even threads which do not use libcurl.  They suggest the singleton pattern when using C++ to enforce that, but maybe we have another way.

Also, if an error does occur I don't think we are leaving the connection in a proper state.  This might call for more use of unwind_actions or similar.  For example, this code segfaults every time for me.


obj = ftp('ftp.gnu.org');
cd(obj, 'gu/emacs');
obj
cd(obj, '/gnu/emacs');


The cause is misspelling in the first cd command which then results in a null directory.


Rik <rik5>
Group administrator
Tue 13 Apr 2021 11:39:04 PM UTC, comment #11: 

I think I see the problem.  There is a loop that iterates over each file, and for each file it actually fetches the content (but discards it), and then queries how much data was transferred to figure out the file size.  That's absurdly inefficient, if I'm right.

Rik <rik5>
Group administrator
Tue 13 Apr 2021 06:32:18 PM UTC, comment #10: 

Yeah, it is complicated.  It needed to be a class for compatibility.  It needed to use an object to manage the ftp connection state that could be copied (the handle/rep implementation) so that the handle could be passed around.  This was in the days before std::shared_ptr and so on.

Initially, the code to interact with cURL was directly in the urlwrite.cc file in libinterp but I moved it to a separate class in liboctave so that it might be used separately from the DEFUNs.

I didn't write the initial code that works with cURL.  But I guess get_fileinfo function was written to just use the cURL functions in a kind of brute-force way that just iterates over the files.  Looking again at the cURL docs, I don't see a faster way to do it using cURL function calls other than to parse the text output of a DIR command because it appears that each query for file size and modification time requires a separate ftp request.  That probably also explains why it takes significantly different amounts of time to get the info about the "ftp://ftp.gnu.org/gnu/emacs" directory.  We are really looking at the network response time, not how fast Octave can run through the loop over the list of files.

John W. Eaton <jwe>
Group administrator
Tue 13 Apr 2021 05:58:49 PM UTC, comment #9: 

This code seems really, really complicated for doing what it does.  Actions start by calling a method function of an old-style classdef object in @ftp dir.  From there it jumps to C++ function in libinterp/corefcn/__ftp__.cc.  Functions there call other functions in url_manager.cc.  Eventually, some calls seem to be split between Octave's own iostream class and url_transfer class which is in liboctave/utils.  And over in liboctave the code is split between a class that just holds a rep pointer and the base class which actually does the work.

Anyways, at the heart of the dir loop are calls to get_fileinfo which is located in liboctave/util/url-transfer.cc.  The function is


    void get_fileinfo (const std::string& filename, double& filesize,
                       time_t& filetime, bool& fileisdir)
    {
      std::string path = pwd ();

      m_url = "ftp://" + m_host_or_url + '/' + path + '/' + filename;
      SETOPT (CURLOPT_URL, m_url.c_str ());
      SETOPT (CURLOPT_FILETIME, 1);
      SETOPT (CURLOPT_HEADERFUNCTION, throw_away);
      SETOPT (CURLOPT_WRITEFUNCTION, throw_away);

      // FIXME
      // The MDTM command fails for a directory on the servers I tested
      // so this is a means of testing for directories.  It also means
      // I can't get the date of directories!

      perform ();
      if (! good ())
        {
          fileisdir = true;
          filetime = -1;
          filesize = 0;

          return;
        }

      fileisdir = false;
      time_t ft;
      curl_easy_getinfo (m_curl, CURLINFO_FILETIME, &ft);
      filetime = ft;
      double fs;
      curl_easy_getinfo (m_curl, CURLINFO_CONTENT_LENGTH_DOWNLOAD, &fs);
      filesize = fs;

      SETOPT (CURLOPT_WRITEFUNCTION, write_data);
      SETOPT (CURLOPT_HEADERFUNCTION, 0);
      SETOPT (CURLOPT_FILETIME, 0);
      m_url = "ftp://" + m_host_or_url;
      SETOPT (CURLOPT_URL, m_url.c_str ());

      // The MDTM command seems to reset the path to the root with the
      // servers I tested with, so cd again into the correct path.  Make
      // the path absolute so that this will work even with servers that
      // don't end up in the root after an MDTM command.
      cwd ('/' + path);
    }


This is performing a large number of cURL operations and is likely where the slow down lies.  The 'cwd' command alone is not costless.

Rik <rik5>
Group administrator
Tue 13 Apr 2021 05:38:41 PM UTC, comment #8: 

I checked in a change just to remove the unnecessary input argument checks here (http://hg.savannah.gnu.org/hgweb/octave/rev/bb64fc1ef1ab).

Rik <rik5>
Group administrator
Tue 13 Apr 2021 05:25:18 PM UTC, comment #7: 

I don't think cURL is doing anything fancy.  This might be a case where we want to intervene and parse the output of the single dir() transaction.  From 'help ftp', most of the commands are one-shot transactions:


     ascii       Set transfer type to ascii
     binary      Set transfer type to binary
     cd          Change remote working directory
     close       Close FTP connection
     delete      Delete remote file
     dir         List remote directory contents
     mget        Download remote files
     mkdir       Create remote directory
     mput        Upload local files
     rename      Rename remote file or directory
     rmdir       Remove remote directory


The notable exceptions are mget, mput, and dir which might iterate over an input list.

The first code C++, called by @ftp class, is in libinterp/corefcn/__ftp__.cc.  The interesting bit for the dir command is


DEFMETHOD (__ftp_dir__, interp, args, nargout,
           doc: /* -*- texinfo -*-
@deftypefn {} {} __ftp_dir__ (@var{handle})
Undocumented internal function
@end deftypefn */)
{
  if (args.length () != 1)
    error ("__ftp_dir__: incorrect number of arguments");

  octave::url_handle_manager& uhm = interp.get_url_handle_manager ();

  octave::url_transfer url_xfer = uhm.get_object (args(0));

  if (! url_xfer.is_valid ())
    error ("__ftp_dir__: invalid ftp handle");

  octave_value retval;

  if (nargout == 0)
    url_xfer.dir ();
  else
    {
      string_vector sv = url_xfer.list ();
      octave_idx_type n = sv.numel ();

      if (n == 0)
        {
          string_vector flds (5);

          flds(0) = "name";
          flds(1) = "date";
          flds(2) = "bytes";
          flds(3) = "isdir";
          flds(4) = "datenum";

          retval = octave_map (flds);
        }
      else
        {
          octave_map st;

          Cell filectime (dim_vector (n, 1));
          Cell filesize (dim_vector (n, 1));
          Cell fileisdir (dim_vector (n, 1));
          Cell filedatenum (dim_vector (n, 1));

          st.assign ("name", Cell (sv));

          for (octave_idx_type i = 0; i < n; i++)
            {
              time_t ftime;
              bool fisdir;
              double fsize;

              url_xfer.get_fileinfo (sv(i), fsize, ftime, fisdir);

              fileisdir (i) = fisdir;
              filectime (i) = ctime (&ftime);
              filesize (i) = fsize;
              filedatenum (i) = double (ftime);
            }

          st.assign ("date", filectime);
          st.assign ("bytes", filesize);
          st.assign ("isdir", fileisdir);
          st.assign ("datenum", filedatenum);

          retval = st;
        }
    }

  return retval;
}


First, this is some very old code.  It checks for the correct number of input arguments, but this is a completely internal function.  It is up to the caller to get it right and such a check should be removed.  You can also see that this is old in that the indexing style doesn't cuddle the parenthis for variables like fileisdir.  Again, just an indication that this is old.

As you can see, if nargout is 0 then this just calls url_xfer.dir() which is presumably efficient.  Otherwise, it calls url_xfer.get_fileinfo () for each file.  Presumably, that is a slow call.

Rik <rik5>
Group administrator
Tue 13 Apr 2021 05:21:54 PM UTC, comment #6: 

Parsing the text output from a DIR command will be faster but file times may be less accurate.  Losing HH:MM:SS data for old files in the list is probably less important than getting the result in a reasonable time.

John W. Eaton <jwe>
Group administrator
Tue 13 Apr 2021 04:57:28 PM UTC, comment #5: 

Yes, Dmitri is correct.

It is not slow when just asking for the output to be printed to the terminal as that is just a single ftp server transaction.

When requesting output as a struct, it is getting the list of files (one ftp server interaction) then iterating through the list to get file info from the server.  Is there a more efficient way to use the cURL library to do this job?  How is the cURL library actually getting the file dates and times?  Is it really asking the server for that info directly or is it just parsing the output of a DIR command?  If the latter, then I think we should just be doing that job so it can be done for a set of files all at once after one server interaction.

John W. Eaton <jwe>
Group administrator
Tue 13 Apr 2021 04:54:11 PM UTC, comment #4: 

I shortened the code to not return a structure, just print the data to screen.


tic;
dir (obj);
toc


The result is 0.7 seconds.  So this is only an issue when trying to return a struct.  It might be better to get the text data and then parse it with regexp() and fill the return struct.

Rik <rik5>
Group administrator
Tue 13 Apr 2021 04:42:36 PM UTC, comment #3: 

From the quick look at the code -- it first gets the file list and
than calls lstat for each file. That is definitely slow.

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Tue 13 Apr 2021 04:37:41 PM UTC, comment #2: 

Confirmed on dev as well.  My timing was even worse---190 seconds---than yours.  Was timing always this bad for you, or is this a new regression?  For me, it seems like it has always been there.  I went back to version 3.4.3 and timing was equally bad.

Rik <rik5>
Group administrator
Tue 13 Apr 2021 01:30:44 PM UTC, comment #1: 

Confirm with 6.2.1

Also if you just use "tic; dir(obj); toc" it is 0.5 sec

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Tue 13 Apr 2021 12:44:53 PM UTC, original submission:  

the following commands show that the dir-method for the ftp class is extremely slow


obj = ftp('ftp.gnu.org');
cd(obj, '/gnu/emacs');
tic ;
ddir = dir(obj);
toc


I get 76 seconds, more than 100x with what I get from Matlab (0.6 sec).

Muhali <muhali>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #51285:  bug60387.patch added by rik5 (4KiB - text/x-patch)
file #51265:  ftpgetinfo.cc added by jwe (5KiB - text/x-c++src)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by dasergatskov (Posted a comment)
  • -email is unavailable- added by muhali (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-04-18 rik5 Attached File- Added bug60387.patch, #51285
    2021-04-14 jwe Attached File- Added ftpgetinfo.cc, #51265
    2021-04-13 rik5 CategoryNone Performance
        StatusNone Confirmed
        Release6.1.0 dev

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code