bugGNU Octave - Bugs: bug #63803, Saving causes OOM, crash, and loss...

 
 

bug #63803: Saving causes OOM, crash, and loss of already saved data

Submitter:  Arun Giridhar <arungiridhar>
Submitted:  Tue 14 Feb 2023 12:38:55 AM UTC
   
 
Category:  Interpreter Severity:  4 - Important
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  9.1.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 21 May 2023 10:01:21 AM UTC, comment #41: 

No new regressions have been reported since the changes approximately two weeks ago.

Closing report as fixed (again).

Markus Mützel <mmuetzel>
Group administrator
Tue 09 May 2023 06:51:35 PM UTC, comment #40: 

I added proper commit messages, left deprecation messages for the "old" `same_file`, and pushed the patches here:
https://hg.savannah.gnu.org/hgweb/octave/rev/3c608abd55f5
https://hg.savannah.gnu.org/hgweb/octave/rev/a4fbcbaa0aa2

If we prefer different behavior, we can still change that.

Marking as ready for test.

Markus Mützel <mmuetzel>
Group administrator
Sat 06 May 2023 04:17:58 PM UTC, comment #39: 

The attached patch "bug63803-rename-existing-target.patch" implements the behavior outlined in comment #38.
For that, `same_file` needed to be moved from liboctinterp to liboctave. That part is done in "bug63803-same_file-v0.patch".

With those patches, the test suite passes for me again on Windows.

Is that a reasonable approach?


(file #54707, file #54708)

Markus Mützel <mmuetzel>
Group administrator
Sat 06 May 2023 03:02:49 PM UTC, comment #38: 

I started up my Ubuntu machine and ran the following tests for completeness:

>> mkdir file1
>> mkdir file2
>> rename file1 file2
>> mkdir file1
>> fid = fopen('file1/a', 'w'); fclose(fid);
>> fid = fopen('file2/b', 'w'); fclose(fid);
>> rename file1 file2
error: rename: operation failed: Directory not empty


Tbh, the behavior of `std::rename` on Windows seems to be a lot more consistent than its behavior on Linux.

On Windows, the behavior is: Fail if the target already exists. Point.

On Linux, the behavior seems to be:
If the source is a regular file and the target is a regular file, overwrite the target.
If the source is a regular file and the target is a folder, fail.
If the source is a folder and the target is a regular file, fail.
If the source is a folder and the target is an empty folder, overwrite the target.
If the source is a folder and the target is a non-empty folder, fail.

While there might be a motivation to this mess, it still looks like a mess to me...

Imho, a reasonable compromise to implement a common behavior across platforms for Octave might be:
If the source is a regular file and the target is a regular file, overwrite the target.
In all other cases where a target already exists, fail.

It would cover the issue here while keeping the logic still pretty straight.

Markus Mützel <mmuetzel>
Group administrator
Sat 06 May 2023 01:51:35 PM UTC, comment #37: 


octave:1> ls -gG
total 0
octave:2> fid = fopen('file1', 'w'); fclose(fid);
octave:3> mkdir('file2');
octave:4> rename('file1', 'file2')
error: rename: operation failed: Is a directory
octave:5> ls -gG
total 4
-rw-r--r-- 1    0 May  6 09:49 file1
drwxr-xr-x 2 4096 May  6 09:49 file2
octave:6> mkdir('file3');
octave:7> fid = fopen('file4', 'w'); fclose(fid);
octave:8> rename('file3', 'file4')
error: rename: operation failed: Not a directory
octave:9> ls -gG
total 8
-rw-r--r-- 1    0 May  6 09:49 file1
drwxr-xr-x 2 4096 May  6 09:49 file2
drwxr-xr-x 2 4096 May  6 09:49 file3
-rw-r--r-- 1    0 May  6 09:50 file4
octave:10>


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sat 06 May 2023 08:56:03 AM UTC, comment #36: 

Thanks for testing.

What is the expected behavior for something like the following (i.e., the target file is an existing directory or vice versa)?

fid = fopen('file1', 'w'); fclose(fid);
mkdir('file2');
rename('file1', 'file2')

mkdir('file3');
fid = fopen('file4', 'w'); fclose(fid);
rename('file3', 'file4')


Markus Mützel <mmuetzel>
Group administrator
Fri 05 May 2023 05:12:42 PM UTC, comment #35: 

Also works with clang/libc++ 16.0.2

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Fri 05 May 2023 04:51:28 PM UTC, comment #34: 

On linux (gcc/libstdc++) seems to work:

octave:1> fid = fopen('file1', 'w'); fprintf(fid, "%f",rand(10)); fclose(fid);
octave:2> ls -go file*
-rw-r--r-- 1 800 May  5 12:48 file1
octave:3> fid = fopen('file2', 'w'); fprintf(fid, "%f",rand(20)); fclose(fid);
octave:4> ls -go file*
-rw-r--r-- 1  800 May  5 12:48 file1
-rw-r--r-- 1 3200 May  5 12:49 file2
octave:5> rename('file1', 'file2')
octave:6> ls -go file*
-rw-r--r-- 1 800 May  5 12:48 file2


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Fri 05 May 2023 04:37:32 PM UTC, comment #33: 

The following also fails with the same error on Windows:

>> fid = fopen('file1', 'w'); fclose(fid);
>> fid = fopen('file2', 'w'); fclose(fid);
>> rename('file1', 'file2')
error: rename: operation failed: File exists


But the following succeeds:

>> fid = fopen('file1', 'w'); fclose(fid);
>> fid = fopen('file2', 'w'); fclose(fid);
>> movefile('file1', 'file2')
>>


I haven't checked on Linux.
Does `rename` succeed or fail when the target file already exists on that platform?

Markus Mützel <mmuetzel>
Group administrator
Thu 04 May 2023 05:55:44 PM UTC, comment #32: 

I believe I ran into an issue caused by this change:
First came across this when running `test addpref` on Windows:

>> test addpref
***** test
 HOME = getenv ("HOME");
 tmpdir = tempname ();
 save_default_options ("-binary", "local");
 unwind_protect
   mkdir (tmpdir);
   setenv ("HOME", tmpdir);

   addpref ("group1", "pref1", [1 2 3]);
   assert (getpref ("group1", "pref1"), [1 2 3]);

   addpref ("group2", {"prefA", "prefB"}, {"StringA", {"StringB"}});
   assert (getpref ("group2", "prefA"), "StringA");
   assert (getpref ("group2", "prefB"), {"StringB"});

   fail ('addpref ("group1", "pref1", 4)', ...
         "preference pref1 already exists in GROUP group1");
   fail ('setpref ("group1", {"p1", "p2"}, 1)', ...
         "size mismatch for PREF and VAL");
   fail ('addpref ("group2", {"prefC", "prefA"}, {1, 2})',
         "preference prefA already exists in GROUP group2");

 unwind_protect_cleanup
   unlink (fullfile (tmpdir, ".octave_prefs"));
   rmdir (tmpdir);
   if (isempty (HOME))
     unsetenv ("HOME");
   else
     setenv ("HOME", HOME);
   endif
 end_unwind_protect
!!!!! test failed
rmdir: operation failed: Directory not empty


Checking that temporary folder, there is a file named `.octave_prefs.saving_in_progress` that causes the error when trying to remove the directory.

I wonder why that isn't an issue on Linux. Does `rename` silently  override an existing file on that platform?

The documentation for `std::rename` says:
https://en.cppreference.com/w/cpp/io/c/rename

> If new_filename exists, the behavior is implementation-defined.


Maybe, we'd need to check if the "target" file already exists and unlink it explicitly before renaming.

Should that be done only for this use case? Or should we change that further down inside `octave::sys::rename`?

Markus Mützel <mmuetzel>
Group administrator
Mon 27 Feb 2023 09:25:41 PM UTC, comment #31: 

Tentatively closing as fixed again. Will reopen if there are reports of problems.

Arun Giridhar <arungiridhar>
Group Member
Fri 24 Feb 2023 02:48:06 PM UTC, comment #30: 

Pushed a fix for the append mode here: https://hg.savannah.gnu.org/hgweb/octave/rev/318dbb0ce30d

Marking as ready for test.

Arun Giridhar <arungiridhar>
Group Member
Thu 23 Feb 2023 01:14:44 PM UTC, comment #29: 

the changeset that implemented that feature in the gui is here:
http://hg.savannah.gnu.org/hgweb/octave/rev/2dc31151ca27

Nicholas Jankowski <nrjank>
Group Member
Thu 23 Feb 2023 01:05:27 PM UTC, comment #28: 

The technique of writing to a temp file and then renaming that as the desired file breaks the case where "-append" is passed as an input option.

Reopening as in progress.

1. Should we do the temp file technique only for cases without "-append"?

2. Or should we copy the old file (if it exists) to the temp file, write to it, which will preserve "-append" behavior if given, then rename the temp file at the end?

Was this solved for the GUI case? Where is that code located?

Arun Giridhar <arungiridhar>
Group Member
Fri 17 Feb 2023 01:12:53 AM UTC, comment #27: 

Seems to be much better now after everyone's patches. No memory spikes, more reliable against file corruption.

Tentatively closing as fixed. If there are problems that crop up in the next few weeks it can be reopened.

Arun Giridhar <arungiridhar>
Group Member
Thu 16 Feb 2023 04:04:30 PM UTC, comment #26: 

This report just keeps on spawning changesets.  Here is another one where I eliminate a call to bool_array_value and just use the member variable holding the data directly as proposed in comment #13: http://hg.savannah.gnu.org/hgweb/octave/rev/80eff88db003

Rik <rik5>
Group administrator
Thu 16 Feb 2023 02:14:28 PM UTC, comment #25: 

Right, I meant that where we call std::rename, we could use the gnulib module to provide a replacement on systems where it is needed.  That doesn't change what we do prior to calling std::rename.

But I see now that we are calling _wrename on Windows and gnulib doesn't use that.

If I understand correctly, the problems that the gnulib module fixes are to work properly on Windows, on which the system rename function will not replace an existing destination file, and on other systems, to fix problems with names that have trailing slash characters.  So I guess those seem like low priority things for Octave.

John W. Eaton <jwe>
Group administrator
Thu 16 Feb 2023 01:51:38 PM UTC, comment #24: 

Gnulib doesn't support non-ASCII characters on Windows. That's why we have our own implementations for file system operations.
If the gnulib functions fix issues on platforms other than Windows, we could use them for those.

Markus Mützel <mmuetzel>
Group administrator
Thu 16 Feb 2023 01:24:02 PM UTC, comment #23: 

There is also a gnulib module for rename that we aren't currently using.  Should we?

John W. Eaton <jwe>
Group administrator
Thu 16 Feb 2023 12:14:51 PM UTC, comment #22: 

Just a generic note about the C++17 filesystem library: Even if we required C++17, current implementations are lacking in some regards (e.g., [1] or [2], or the list in [3] for the libstdc++ implementation). So, imho, we shouldn't use it (yet).

[1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99333
[2]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97813
[3]: https://gcc.gnu.org/bugzilla/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=WAITING&bug_status=REOPENED&bug_status=VERIFIED&cf_known_to_fail_type=allwords&cf_known_to_work_type=allwords&query_format=advanced&short_desc=filesystem&short_desc_type=allwordssubstr

Markus Mützel <mmuetzel>
Group administrator
Thu 16 Feb 2023 01:35:01 AM UTC, comment #21: 

I did:


octave:1> load "データ.bin"
octave:2> whos
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name        Size                     Bytes  Class
  ====   ====        ====                     =====  =====
         a       10000x10000              800000000  double
         b       10000x10000              800000000  double
         c       50000x50000            20000000000  double

Total is 2700000000 elements using 21600000000 bytes

octave:3> d = zeros(5e4);
octave:4> save -binary  "データ.bin" a b c d
Killed
(I killed -9 it manually)

$ ./run-octave
GNU Octave, version 9.0.0

octave:1> load "データ.bin"
octave:2> whos
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name        Size                     Bytes  Class
  ====   ====        ====                     =====  =====
         a       10000x10000              800000000  double
         b       10000x10000              800000000  double
         c       50000x50000            20000000000  double

Total is 2700000000 elements using 21600000000 bytes

octave:3>


So, success?

Note it did require quotes around "データ.bin" otherwise I get an
error:


octave:6> load データ.bin
error: load: unable to find file .bin


(Works w/o quotes for a file with ascii name.)
Not sure if this is another, non-related, bug.


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Thu 16 Feb 2023 01:07:57 AM UTC, comment #20: 

I pushed this changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/c82c5f7a1579

The octave::sys::file_ops::rename function does appear to handle non-ascii filenames, at least on Windows systems.  If it doesn't do it properly on all systems, then we should fix that function, not just in one function in load-save.cc.

John W. Eaton <jwe>
Group administrator
Thu 16 Feb 2023 12:57:45 AM UTC, comment #19: 

Does it work on non-ascii file names?


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Thu 16 Feb 2023 12:44:48 AM UTC, comment #18: 

That was odd. I tried it out with both gcc and clang before pushing it, but it seems that it works only as an incremental build with clang if it was already configured with an earlier version but doesn't work with a fresh configure with clang.

I've pushed a variation that uses the cstdio rename() function which does build from scratch with both gcc and clang, so hopefully it's more portable.

From what I understand, the std::filesystem abstraction accommodates network drive paths in a portable way. I do not know if that works for the cstdio rename() function as well. Either way, more testing is always good.

Arun Giridhar <arungiridhar>
Group Member
Thu 16 Feb 2023 12:23:05 AM UTC, comment #17: 

std::filesystem is c++17.

But if all you need is to rename a file, I think we already have a function for that.

John W. Eaton <jwe>
Group administrator
Wed 15 Feb 2023 11:57:22 PM UTC, comment #16: 
Dmitri A. Sergatskov <dasergatskov>
Wed 15 Feb 2023 11:01:42 PM UTC, comment #15: 

I pushed a change https://hg.savannah.gnu.org/hgweb/octave/rev/354ed032ba50 to write to a temp file first and then rename it to the desired filename. This works for me exactly as intended, and it doesn't use extra disk space. I've only used standard C++ functions, but it needs testing from CI for cross-platform behavior.

Please stress test this function if you can. It needs to be reliable so that saving data is robust.

Example test: save a large amount of data to a file test.mat, then save more variables to the same file, but while it is writing it, kill the Octave process (e.g. `kill -9 octave`). Then restart Octave and load test.mat -- it should be intact and all the data should be as it was with the last successful save.

Arun Giridhar <arungiridhar>
Group Member
Wed 15 Feb 2023 02:17:04 AM UTC, comment #14: 

In this case it won't make much difference since we are using data to get a const bool pointer to the data and no copy will be made.  But we can avoid the need to create second boolNDArray handle to the same underlying data by using m_matrix directly.  We might as well do that.

John W. Eaton <jwe>
Group administrator
Wed 15 Feb 2023 01:11:19 AM UTC, comment #13: 

One last question.  In ov-bool-mat.cc the code is now


  boolNDArray m = bool_array_value ();
  const bool *mtmp = m.data ();
  octave_idx_type nel = m.numel ();
  os.write (reinterpret_cast<const char*> (mtmp), nel);


But, octave_bool_matrix is


class
octave_bool_matrix : public octave_base_matrix<boolNDArray>


and the member variable "m_matrix" is already of type boolNDArray.  Thus, the definition of bool_array_value() is


  boolNDArray bool_array_value (bool = false) const
  { return m_matrix; }


So, instead of creating the temporary boolNDArray value m, we could just write


const bool *mtmp = m_matrix.data ();


I suppose the disadvantage would be if we changed the underlying data type of octave_base_matrix then this approach might not work, while the original would.  Of course, I don't think we would be changing data types all that frequently and it would probably be a big deal for us to do so and we would scrub the code base for these things anyways.

Alternatively, if we keep the existing structure, would it be better to use either copy constructor or initialization rather than an assignment operator?  It seems to me that assignment operator will first call boolNDArray constructor with zero arguments, and then call assignment operator.  Maybe optimizing compiler edits out the first empty construction, but if not that is just wasted effort.

So, two additional proposals might be


  boolNDArray m (bool_array_value ());
  const bool *mtmp = m.data ();


or


  boolNDArray m {bool_array_value ()};
  const bool *mtmp = m.data ();


Thoughts?

Rik <rik5>
Group administrator
Wed 15 Feb 2023 12:56:43 AM UTC, comment #12: 

On loading, we do want to write directly into the array so fortran_vec is exactly what we want.

Rik <rik5>
Group administrator
Wed 15 Feb 2023 12:09:48 AM UTC, comment #11: 

Looks like we crossed edits. I hadn't seen the discussion when testing and pushing my changes but I think it's merged now.

Is there anything we can do for the load files or is fortran_vec unavoidable there?

Arun Giridhar <arungiridhar>
Group Member
Tue 14 Feb 2023 11:48:07 PM UTC, comment #10: 

It looks like the same type of copy is made in octave_bool_matrix::save_hdf5.

John W. Eaton <jwe>
Group administrator
Tue 14 Feb 2023 11:38:57 PM UTC, comment #9: 

There are 44 instances of save_binary in libinterp.  The only one that looked interesting was in ov-str-mat.h which I fixed here: http://hg.savannah.gnu.org/hgweb/octave/rev/da5b50dc82b0.

Rik <rik5>
Group administrator
Tue 14 Feb 2023 10:58:35 PM UTC, comment #8: 

@Arun: I can confirm that memory usage is very small now.  I also discovered the '-t' argument to free so I didn't even need to use a while loop in the shell.

This change definitely needs to be propagated across all of the various save_XXX routines.  A lot of code used to use the fortran_vec() method to get a pointer to the data contained, and I don't think we were always careful about using "const" modifier even when we knew it was read-only data.  The issue is also present in the load_binary routines.  This is the code for load_binary in ov-bool-mat.cc


  octave_idx_type nel = dv.numel ();
  OCTAVE_LOCAL_BUFFER (char, htmp, nel);
  if (! is.read (htmp, nel))
    return false;
  boolNDArray m(dv);
  bool *mtmp = m.fortran_vec ();
  for (octave_idx_type i = 0; i < nel; i++)
    mtmp[i] = (htmp[i] ? 1 : 0);
  m_matrix = m;


Arguably, one still might want to keep it this way because the file is first read into a temporary variable (htmp) and only then copied to its true destination.  If the read fails, then the destination is not harmed.  If Octave read directly into destination then a partial, corrupted read would result in a partial, corrupted variable in Octave.

Rik <rik5>
Group administrator
Tue 14 Feb 2023 08:30:55 PM UTC, comment #7: 

Pushed to https://hg.savannah.gnu.org/hgweb/octave/rev/f14aea577cc5

Marking as ready for test (for that part alone).

The rest of this is a work in progress (making the save process more robust with temp files, going through all save_binary functions, and replacing fortran_vec with data where possible.)

Arun Giridhar <arungiridhar>
Group Member
Tue 14 Feb 2023 07:00:40 PM UTC, comment #6: 

I see what you mean. Changing that line to read:

const bool *mtmp = m.data ();

eliminates one copy. For the same 5 GB array:

octave:4> save -binary test3.mat
../libinterp/octave-value/ov-bool-mat.cc:369 Usage: 5571059712
../libinterp/octave-value/ov-bool-mat.cc:374 Usage: 5571059712
../libinterp/octave-value/ov-bool-mat.cc:379 Usage: 5571059712
../libinterp/octave-value/ov-bool-mat.cc:384 Usage: 5571059712
../libinterp/octave-value/ov-bool-mat.cc:389 Usage: 10571063296
../libinterp/octave-value/ov-bool-mat.cc:395 Usage: 10571063296
../libinterp/octave-value/ov-bool-mat.cc:400 Usage: 10571063296


Further eliminating htmp and writing the file directly as a reinterpret_cast from mtmp removes all temporaries:

octave:1> arr = false (1e5, 5e4);
octave:2> whos
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name        Size                     Bytes  Class
  ====   ====        ====                     =====  =====
         arr    100000x50000             5000000000  logical

Total is 5000000000 elements using 5000000000 bytes

octave:3> nnz (arr)
ans = 0
octave:4> save -binary test4.mat
../libinterp/octave-value/ov-bool-mat.cc:369 Usage: 5571059712
../libinterp/octave-value/ov-bool-mat.cc:374 Usage: 5571059712
../libinterp/octave-value/ov-bool-mat.cc:379 Usage: 5571059712
../libinterp/octave-value/ov-bool-mat.cc:384 Usage: 5571059712
../libinterp/octave-value/ov-bool-mat.cc:389 Usage: 5571059712


The files test3.mat and test4.mat are identical, so we don't need the htmp intermediate to write booleans as characters.

$ diff test3.mat test4.mat
$


Here's a fix for this function, which eliminates all temporaries:

diff -r 3e4e74ad8fd7 libinterp/octave-value/ov-bool-mat.cc
--- a/libinterp/octave-value/ov-bool-mat.cc     Mon Feb 13 11:10:00 2023 -0800
+++ b/libinterp/octave-value/ov-bool-mat.cc     Tue Feb 14 13:57:31 2023 -0500
@@ -362,14 +362,9 @@ octave_bool_matrix::save_binary (std::os
     }

   boolNDArray m = bool_array_value ();
-  bool *mtmp = m.fortran_vec ();
+  const bool *mtmp = m.data ();
   octave_idx_type nel = m.numel ();
-  OCTAVE_LOCAL_BUFFER (char, htmp, nel);
-
-  for (octave_idx_type i = 0; i < nel; i++)
-    htmp[i] = (mtmp[i] ? 1 : 0);
-
-  os.write (htmp, nel);
+  os.write (reinterpret_cast<const char*> (mtmp), nel);

   return true;
 }


Arun Giridhar <arungiridhar>
Group Member
Tue 14 Feb 2023 06:14:57 PM UTC, comment #5: 

I'd guess it triples memory usage for the same reason we usually have these problems:  the reference count of the object is greater than one and fortran_vec is a non-const method that forces a copy to be made.  We should be using the const "data" method here.  It's probably worth reviewing all the uses of non-const fortran_vec and data methods to see whether they can be const instead.  BTW, if someone wants to look at this, I find that it helps to use the OCTAVE_DEPRECATED macro to locate calls to specific functions.  It's usually easier and more accurate than grep for me.

John W. Eaton <jwe>
Group administrator
Tue 14 Feb 2023 05:43:44 PM UTC, comment #4: 

I instrumented the octave_bool_matrix::save_binary function in the ov-bool-mat.cc file as shown in the attached hg diff. If you use it, add this to your configure line when you build Octave: LIBS="-lprocps".

The big result is that it triples the memory usage, not just double. So when I was trying to write about 7.5 GB in comment #0, it was using roughly 23 GB of memory, which was a little beyond my RAM + swap capacity after taking into account the memory already used to run OS services, hence an OOM crash in the middle of the save, corrupting the saved file.


octave:1> arr = false (1e5, 5e4);

octave:2> whos
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name        Size                     Bytes  Class
  ====   ====        ====                     =====  =====
         arr    100000x50000             5000000000  logical

Total is 5000000000 elements using 5000000000 bytes

octave:3> nnz (arr)
ans = 0

octave:4> save -binary test2.mat
../libinterp/octave-value/ov-bool-mat.cc:369 Usage: 5571063808
../libinterp/octave-value/ov-bool-mat.cc:374 Usage: 5571063808
../libinterp/octave-value/ov-bool-mat.cc:379 Usage: 10571067392
../libinterp/octave-value/ov-bool-mat.cc:384 Usage: 10571067392
../libinterp/octave-value/ov-bool-mat.cc:389 Usage: 15571070976
../libinterp/octave-value/ov-bool-mat.cc:395 Usage: 15571070976
../libinterp/octave-value/ov-bool-mat.cc:400 Usage: 15571070976

octave:5> whos
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name        Size                     Bytes  Class
  ====   ====        ====                     =====  =====
         ans         1x1                          8  double
         arr    100000x50000             5000000000  logical

Total is 5000000001 elements using 5000000008 bytes

octave:6> exit


To decode the line numbers above, one copy is created by this line:

bool *mtmp = m.fortran_vec ();

and another by this line:

OCTAVE_LOCAL_BUFFER (char, htmp, nel);


To build on nrjank's points in comment #1:

  • For robustness, can we write to a temporary and then rename it to the given filename, as opposed to writing to that file directly? This might consume more disk space during the save, but that's better and more robust than tripling the RAM usage during the save and crashing.


  • There are some 44 or so save_binary functions for all the different data types. Should each of them be fixed individually or is there a way to fix multiple types at once to avoid the creation of temporaries?


  • I cannot figure out why it creates two temporaries. Shouldn't mtmp be just a pointer to the fortran_vec, not yet another copy of it? Why does calling fortran_vec() trigger a copy to be created?



(file #54357)

Arun Giridhar <arungiridhar>
Group Member
Tue 14 Feb 2023 06:10:02 AM UTC, comment #3: 

I don't see a crash, but the system I ran it on has 128GB of memory.

You probably want to look at octave_bool_matrix::save_binary in ov-bool-mat.cc.  I don't remember for sure now, but I suspect the reason that there is a copy of the data to a local buffer there is so that the only values written are 1 and 0 in case the compiler might store some other values for bool TRUE/FALSE.  That's very old code, from what I can tell.  It may not be needed now if there is a guarantee about the values of bool TRUE/FALSE in standard C++ that was not present way back in the olden times.  Even if we do need to ensure 1/0 values, we could do this with a loop over a smaller temporary buffer size.

John W. Eaton <jwe>
Group administrator
Tue 14 Feb 2023 05:01:42 AM UTC, comment #2: 

My guess is that the other variables have nothing to do with it and it is 'arr' which is the problem.  The only variable that was saved was 'adj', but this is also the only variable that precedes 'arr' when listed alphabetically.  My guess is that Octave has a for loop over the variables and it is in the same order as displayed by 'whos'.

If this is correct, you could probably just test with the 'arr' and 'lst' variables.  That might help narrow things down.

In one shell window I ran


while (1)
  free -h
  sleep 1
end


In another shell I ran octave as a CLI and then executed


y = false (1e5, 2e4);
y(1:3:end) = true;


This variable takes up 2 GB.  Then I tried saving with


save -binary test.mat y


Memory usage peaks at 4 GB during the save which would imply two copies of the variable are made during the save process.

Rik <rik5>
Group administrator
Tue 14 Feb 2023 12:51:35 AM UTC, comment #1: 

Separate issues - (1) why the crash, (2) why losing previously saved data

In the editor, we had a similar problem. A crash diluting save away wiping previously saved data.  fixed by writing to a temp file and only overwriting after a successful and verified write. It was fairly straightforward in the qt gui. Not sure if something similar can be done for writing mat files

Nicholas Jankowski <nrjank>
Group Member
Tue 14 Feb 2023 12:38:55 AM UTC, original submission:  

This is on: GNU Octave Version: 9.0.0 (hg id: 3e4e74ad8fd7)

Prep work: Create a few arrays, save to file, and exit Octave:

adj = ~eye(36);
edges = reshape (1:120, 60, 2);
lst = false (5735500, 60);
ncells = 36;
nedges = 60;
whos
save -binary test.mat


Output is normal:

octave:6> whos
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name         Size                     Bytes  Class
  ====   ====         ====                     =====  =====
         adj         36x36                      1296  logical
         edges       60x2                        960  double
         lst    5735500x60                 344130000  logical
         ncells       1x1                          8  double
         nedges       1x1                          8  double

Total is 344131418 elements using 344132272 bytes

octave:7> save -binary test.mat
octave:8> exit


Then restart Octave, load that file, then create a new large variable, and try to save everything to the same file. I have 16 GB RAM, and I can comfortably create a 7.5 GB array with no second thought:

load test.mat
whos
arr = false (36, 36, rows(lst));
arr(:, :, 1:2:end) = repmat(mod((1:36)' + (1:36), 2) == 1, 1, 1, size(arr,3)/2);
arr(:, :, 2:2:end) = ~arr(:, :, 1:2:end);
whos
save -binary test.mat


Output:

octave:6> whos
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name         Size                     Bytes  Class
  ====   ====         ====                     =====  =====
         adj         36x36                      1296  logical
         arr         36x36x5735500        7433208000  logical
         edges       60x2                        960  double
         lst    5735500x60                 344130000  logical
         ncells       1x1                          8  double
         nedges       1x1                          8  double

Total is 7777339418 elements using 7777340272 bytes

octave:7> save -binary test.mat
Killed


What happened?! Let's restart Octave and see what happened:

octave:1> load test.mat
octave:2> whos
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name        Size                     Bytes  Class
  ====   ====        ====                     =====  =====
         adj        36x36                      1296  logical

Total is 1296 elements using 1296 bytes


All the other variables have been lost both from RAM and from the saved file.

This actually happened to me in the last 30 minutes. Fortunately I lost only 12 minutes of work and was able to rerun a script that regenerated everything. Had it been more important data this could have been BAD!

Raising importance because of the ease and surprise of data loss right in the act of saving it, even with plenty of memory remaining.

Observations if it helps anyone localize the cause:

  • Monitoring the memory usage through top, Octave somehow uses 16 GB RAM + 8 GB swap, runs out of memory, and is killed. It's not clear why the memory use goes that high for a total of less than 8 GB worth of variables. Looks like copy-on-write happening where it shouldn't.


  • The saving works as intended if the new array "arr" is all-false. But if it's set to a checkerboard pattern like in the case above, it crashes while saving. Not sure why the density of the array causes a crash since it's not even a sparse object, but it empirically does. Maybe something to do with the way it's created? Copy-on-write problem with ranges?


Arun Giridhar <arungiridhar>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #54707:  bug63803-same_file-v0.patch added by mmuetzel (12KiB - application/octet-stream)
file #54708:  bug63803-rename-existing-target.patch added by mmuetzel (1KiB - application/octet-stream)
file #54357:  memory.patch added by arungiridhar (2KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2023-05-21 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2023-05-09 mmuetzel StatusPatch Submitted Ready For Test
    2023-05-06 mmuetzel Attached File- Added bug63803-same_file-v0.patch, #54707
        Attached File- Added bug63803-rename-existing-target.patch, #54708
        StatusReady For Test Patch Submitted
        Operating SystemGNU/Linux Any
    2023-05-04 mmuetzel StatusFixed Ready For Test
        Open/ClosedClosed Open
    2023-02-27 arungiridhar StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2023-02-24 arungiridhar StatusIn Progress Ready For Test
    2023-02-23 arungiridhar StatusFixed In Progress
        Open/ClosedClosed Open
    2023-02-17 arungiridhar StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2023-02-15 arungiridhar StatusReady For Test In Progress
    2023-02-14 arungiridhar StatusPatch Submitted Ready For Test
    2023-02-14 arungiridhar StatusNone Patch Submitted
    2023-02-14 arungiridhar Attached File- Added memory.patch, #54357

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code