[Octave-bug-tracker] [bug #54100] fread using SKIP larger than zero is extremely slow

[Dan Sebald]

Follow-up Comment #6, bug #54100 (project octave):

You could print out the value of input_buf_size to confirm, i.e.,


std::cerr << "IBS: " << input_buf_size << "\n";


then run an example using a non-zero skip value.  But, yeah, I would think the
skip always has to be done in one, let's say, "read group" or "read vector".

True about the cost of function calls, especially since it appears seek() is
an added C++ layer.  The routine is not using the C++ stream library's seekg()
method directly.  Instead, there seems to be a virtual octave::base_stream
class function seek().

Perhaps that is why there's so much activity going on with the stream's
integrity:


        while (is ...


"is" is a reference, not a pointer.  (Does it make sense to check if a
reference is non-null?)  I don't think the C++ library would delete an
existing stream as a result of a read() or seekg() member call, so why keep
checking in the loop?  I'd think that just once prior is fine.  In any case,
the lines with an asterisk below seem superfluous:


            if (is && skip != 0 && nel == block_size)
              {
...
*               if (! is)
*                 break;
              }



if there is a check at the top of the loop on "is" which is the next step in
program flow.

Your prescription sounds good.  Eliminate the skip==0 check and just make it
generic where the skip value happens to be 0.

There's an added benefit to not using such small atomic reads, which is that
the routine is using "new" within the loop to create a list of data chunks. 
It must then at the end of all those read construct a valid octave_value data
object with the routine


        retval = finalize_read (input_buf_list, input_buf_elts, count,
                                nr, nc, input_type, output_type, ffmt);


I think that could be a very large number of hunks which means inefficient
call to "new" all the time plus the list management


input_buf_list.push_back (input_buf);


A better way to do these dynamic memory kinds of things is to start with a
small buffer or "work space", and when that memory area runs out, create one
twice the size and copy the old data to the new data and free the old data. 
That way the size of the workspace remains on the order of the amount of data
being read.  It's actually fairly efficient.  Just as you're suggesting to
make the processing into a streamline operation at the CPU level, block copies
of data are really fast; just set up the CPU for a tight loop that copies data
from one place to another and let it chug away.  (That would be a separate
changeset.)

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?54100>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/