bugGNU Octave - Bugs: bug #59702, dlmread performance

 
 

bug #59702: dlmread performance

Submitter:  Charles Praplan <charprap>
Submitted:  Wed 16 Dec 2020 01:07:59 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Performance
Status:  Fixed Assigned to:  None
Originator Name:  Charles Praplan Open/Closed:  * Closed
Release:  * dev Operating System:  * Microsoft Windows
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 07 Aug 2021 08:31:35 AM UTC, comment #21: 

The change was made on the default branch which will eventually be released as Octave 7.1.
The buildbots on octave.space are building from the stable branch (currently 6.3.1).

Markus Mützel <mmuetzel>
Group administrator
Sat 07 Aug 2021 08:27:28 AM UTC, comment #20: 

Should this modification already be integrated in the daily builds?

I ask this question because I don't see any improvement in octave-2021-08-06-00-41-w64.

Charles Praplan <charprap>
Sat 31 Jul 2021 06:12:27 AM UTC, comment #19: 

Re comment #18: Oh great! I didn't see the changeset. Thanks again!

Anonymous
Fri 30 Jul 2021 08:18:54 PM UTC, comment #18: 

Re comment #17: Isn't that what I did?

Markus Mützel <mmuetzel>
Group administrator
Fri 30 Jul 2021 07:27:36 PM UTC, comment #17: 

Thanks!
It would be better to use "unwind_action" to handle the failure of dlmread because of exception.


char* old = std::setlocale(LC_ALL, 0);
std::string sav = old ? old : "";
std::setlocale(LC_ALL, "C");
unwind_action act ([&sav](){std::setlocale(LC_ALL, sav.c_str ());});


Anonymous
Fri 30 Jul 2021 05:19:45 PM UTC, comment #16: 

@Markus: I assume you had a look at bug #60984?  It would be worth checking whether this rather simple solution improves performance there as well.

Rik <rik5>
Group administrator
Fri 30 Jul 2021 03:08:07 PM UTC, comment #15: 

Sorry for the long reaction time.

With the changes proposed in comment #14, I see approximately the same performance that we had in Octave 5.2.0.

This might also improve performance on other platforms than Windows. But I didn't see any significant difference on Ubuntu with or without the patch.

I pushed a change here:
https://hg.savannah.gnu.org/hgweb/octave/rev/2420f8f62ebe

Maybe, we could do something similar at other points in the code where we are reading floating point values from a text input stream repeatedly.

Marking as ready for test.

Markus Mützel <mmuetzel>
Group administrator
Wed 27 Jan 2021 07:32:49 AM UTC, comment #14: 

Inspired by libstdc++ source[1] this may fix the bug:

At the beginning of 'dlmread' from libinterp/corefcn/dlmread.cc add the following lines:

char* old = std::setlocale(LC_ALL, 0);
std::string sav = old ? old : "";
std::setlocale(LC_ALL, "C");


and at the end of the function add this line:

std::setlocale(LC_ALL, sav.c_str ());


[1] https://github.com/gcc-mirror/gcc/blob/16e2427f50c208dfe07d07f18009969502c25dc8/libstdc%2B%2B-v3/config/locale/generic/c_locale.cc#L59

Anonymous
Tue 26 Jan 2021 01:08:28 PM UTC, comment #13: 

I already contributed to octave in the signal processing toolbox.
If I were able to implement such corrections, I would. Unfortunately I'm not a SW engineer...
To be used widely, Octave has to be efficient also with large amounts of data and also under windows.
So I hope that somebody will be able to correct this performance lost, especially since the solution seems to have been found.
Thank you for your help.

Charles Praplan <charprap>
Mon 25 Jan 2021 03:48:27 PM UTC, comment #12: 

Octave is an open source project developed primarily by a community of volunteers.
You are welcome to contribute, too. I'm sure someone will be happy to review any patches you'll provide.

Markus Mützel <mmuetzel>
Group administrator
Mon 25 Jan 2021 02:58:26 PM UTC, comment #11: 

Is something planned to correct this annoying problem?
As I understood, the cause in known.

Charles Praplan <charprap>
Mon 21 Dec 2020 06:48:06 PM UTC, comment #10: 

double-conversion is present on Arch, and is already required by qt5-base and vtk.

Anonymous
Thu 17 Dec 2020 12:42:22 PM UTC, comment #9: 

It is also available on macos via macports


$ port list double-conversion
double-conversion              @3.1.0          devel/double-conversion
$ port contents double-conversion
Port double-conversion contains:
  /opt/local/include/double-conversion/bignum.h
  /opt/local/include/double-conversion/cached-powers.h
  /opt/local/include/double-conversion/diy-fp.h
  /opt/local/include/double-conversion/double-conversion.h
  /opt/local/include/double-conversion/fast-dtoa.h
  /opt/local/include/double-conversion/fixed-dtoa.h
  /opt/local/include/double-conversion/ieee.h
  /opt/local/include/double-conversion/strtod.h
  /opt/local/include/double-conversion/utils.h
  /opt/local/lib/cmake/double-conversion/double-conversionConfig.cmake
  /opt/local/lib/cmake/double-conversion/double-conversionConfigVersion.cmake
  /opt/local/lib/cmake/double-conversion/double-conversionTargets-macports.cmake
  /opt/local/lib/cmake/double-conversion/double-conversionTargets.cmake
  /opt/local/lib/libdouble-conversion.3.0.0.dylib
  /opt/local/lib/libdouble-conversion.dylib


Carlo de Falco <cdf>
Group Member
Wed 16 Dec 2020 08:58:22 PM UTC, comment #8: 

double-conversion is also packaged for openSUSE.

Atri Bhattacharya <badshah400>
Wed 16 Dec 2020 08:02:55 PM UTC, comment #7: 

It is packaged on RedHat/Fedora and related:


rpm -ql double-conversion-devel
/usr/include/double-conversion
/usr/include/double-conversion/bignum.h
/usr/include/double-conversion/cached-powers.h
/usr/include/double-conversion/diy-fp.h
/usr/include/double-conversion/double-conversion.h
/usr/include/double-conversion/fast-dtoa.h
/usr/include/double-conversion/fixed-dtoa.h
/usr/include/double-conversion/ieee.h
/usr/include/double-conversion/strtod.h
/usr/include/double-conversion/utils.h
/usr/lib64/cmake/double-conversion
/usr/lib64/cmake/double-conversion/double-conversionConfig.cmake
/usr/lib64/cmake/double-conversion/double-conversionConfigVersion.cmake
/usr/lib64/cmake/double-conversion/double-conversionTargets-noconfig.cmake
/usr/lib64/cmake/double-conversion/double-conversionTargets.cmake
/usr/lib64/libdouble-conversion.so


Dmitri
--

Dmitri A. Sergatskov <dasergatskov>
Wed 16 Dec 2020 07:45:42 PM UTC, comment #6: 

I would generally prefer to use standard C++ interfaces and features but if there are problems with those then I don't object to using something else.  In mxe-octave we already build the double-conversion library for Qt, so it shouldn't cause trouble there.  It's also packaged for Debian.  What about other systems?

John W. Eaton <jwe>
Group administrator
Wed 16 Dec 2020 06:01:25 PM UTC, comment #5: 

When I run the same code in Octave 5.2 and Octave 6.1, `setlocale` is called 24 times in Octave 5.2 vs. 66 times in Octave 6.1.
That difference is probably the main reason for the performance difference between the two versions.

But instead of trying to reduce the number of times we use the `>>` operator (which I think is the reason for the slow code path), it might be worth considering replacing it by a more performant implementation altogether.

Would it be okay if we added a dependency on Google's double-conversion library?
https://github.com/google/double-conversion

Markus Mützel <mmuetzel>
Group administrator
Wed 16 Dec 2020 05:24:35 PM UTC, comment #4: 

Other projects seem to have had a similar experience:
https://github.com/openscad/openscad/issues/2744

It looks like they opted to use double-conversion instead of standard library functions because of the quite significant performance benefit.

Markus Mützel <mmuetzel>
Group administrator
Wed 16 Dec 2020 04:22:57 PM UTC, comment #3: 

I ran the line with `dlmread` in a loop while I attached a profiler to the Octave process.

Most of the time is spent in the function `setlocale` in the Microsoft Visual C Run Time (msvcrt).
The innermost function in one of Octave's libraries that calls this function is `Z20octave_read_fp_valueIdEdRSi`. I tried to demangle that symbol [1] but was unsuccessful.
It is probably an instance of the templated function here:
https://hg.savannah.gnu.org/hgweb/octave/file/ed1a8af4242f/liboctave/util/lo-utils.cc#l242

The top of the call stack reads the following in Very Sleepy CS:

setlocale        msvcrt        [unknown]        0        0x7ffd9c349c3d
ZSt14__convert_to_vIdEvPKcRT_RSt12_Ios_IostateRKPi        libstdc++-6        [unknown]        0        0xdd4e1c
ZNKSt7num_getIcSt19istreambuf_iteratorIcSt11char_traitsIcEEE6do_getES3_S3_RSt8ios_baseRSt12_Ios_IostateRd        libstdc++-6        [unknown]        0        0xd359f9
ZNSi10_M_extractIdEERSiRT_        libstdc++-6        [unknown]        0        0xd59a7e
Z20octave_read_fp_valueIdEdRSi        liboctave-8        [unknown]        0        0x6d2325aa
Z8FdlmreadRN6octave11interpreterERK17octave_value_listi        liboctinterp-8        [unknown]        0        0x6f93e056


It's not (yet) clear to me why that would need a call of `setlocale`, why it is so slow, and why it looks like it was faster in the past.
Might be caused by a different version of GCC that we used for compiling Octave 5.2 and Octave 6.1.

[1]: https://demangler.com/

Markus Mützel <mmuetzel>
Group administrator
Wed 16 Dec 2020 04:02:11 PM UTC, comment #2: 

I understand the difference between linux and windows.
But how can you explain the difference between Octave 5 and 6 on win10?

Charles Praplan <charprap>
Wed 16 Dec 2020 02:07:10 PM UTC, comment #1: 

It is got to be one of the windows-related issues. I got 1.7sec on
win10 and 0.057sec on linux (running off the same USB thumb drive).

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Wed 16 Dec 2020 01:07:59 PM UTC, original submission:  

I have noticed that Octave 6.1.0 seems to be slower than Octave 5.2.0.
This is already visible at startup when loading the prolog, but this is not the issue.

Here is another example using dlmread. Below is a summary of the time needed to read the appended data file (from a SSD drive) with the appended script.

               Small data file    Large data file (100x)
                 (appended)
octave 5.2.0 : 0.502303 seconds   49.4749 seconds
octave 6.1.0 : 1.99895 seconds    213.084 seconds
matlab 2019a : 0.104778 seconds   4.539602 seconds

There is factor 4 between version 5 and 6!

Can somebody explain this?

Charles Praplan <charprap>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #50488:  data_2n_part_50ksamp.csv added by charprap (1MiB - application/vnd.ms-excel)
file #50489:  V6_slower_than_V5_dlmread_test_case.m added by charprap (157B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by cdf (Posted a comment)
  • -email is unavailable- added by badshah400 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by dasergatskov (Posted a comment)
  • -email is unavailable- added by charprap (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-08-05 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2021-07-30 mmuetzel CategoryNone Octave Function
        StatusNone Ready For Test
        Release6.1.0 dev
    2020-12-17 jwe SummaryOctave 6 slower then Octave 5 ? dlmread performance
    2020-12-16 charprap Attached File- Added data_2n_part_50ksamp.csv, #50488
        Attached File- Added V6_slower_than_V5_dlmread_test_case.m, #50489

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code