bugGNU Octave - Bugs: bug #57439, handles to private functions may...

 
 

bug #57439: handles to private functions may fail after "clear functions"

Submitter:  John W. Eaton <jwe>
Submitted:  Wed 18 Dec 2019 06:06:20 PM UTC
   
 
Category:  Interpreter Severity:  5 - Blocker
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  jwe Open/Closed:  * Closed
Release:  * 6.0.90 Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 23 Jun 2020 07:34:29 AM UTC, comment #55: 

This seems to be fixed (across platforms) by the recent improvement jwe made for the implementation of function handles. See this mailing list thread:
https://lists.gnu.org/archive/html/octave-maintainers/2020-06/msg00058.html

Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Sun 03 May 2020 04:24:13 AM UTC, comment #54: 

Markus: We do need to remove the call to canonicalize_file_name in find_private_fcn.  Instead of that, we need to fix function handles to do the right thing when the underlying function has been cleared.  I'm working on that but not done yet.  Investigating this issue revealed a number of other problems with function handles, which led to a nearly complete rewrite of the octave_function_handle class.  I expect to post something about it all on the maintainers list soon and will also comment here when I do.

John W. Eaton <jwe>
Group administrator
Sat 02 May 2020 07:17:54 PM UTC, comment #53: 

For a test, I completely removed the letter case code from canonicalize_file_name:

diff -r 1d090de50a0d liboctave/system/file-ops.cc
--- a/liboctave/system/file-ops.cc        Fri May 01 15:50:24 2020 +0200
+++ b/liboctave/system/file-ops.cc        Sat May 02 21:07:08 2020 +0200
@@ -714,7 +714,6 @@

 #if defined (OCTAVE_USE_WINDOWS_API)
       std::wstring w_tmp;
-      bool strip_marker = true;
       if (retval.empty ())
         {
           // For UNC paths, take the input as is.
@@ -724,39 +723,13 @@
           if (name_backsl.compare (0, 2, "\\\\") == 0)
             {
               w_tmp = u8_to_wstring (name_backsl);
-              strip_marker = false;
               wchar_t canon_path[MAX_PATH];
               if (PathCanonicalizeW (canon_path, w_tmp.c_str ()))
                 w_tmp = std::wstring (canon_path);
             }
-        }
-      else
-        w_tmp = L"\\\\?\\" + u8_to_wstring (retval);

-      if (! w_tmp.empty ())
-        {
-          // Get a more canonical name wrt case and full names
-          // FIXME: To make this work on partitions that don't store short file
-          // names, use FindFirstFileW on each component of the path.
-          // Insufficient access permissions on parent folders might make this
-          // tricky.
-
-          // Parts of the path that wouldn't fit into a short 8.3 file name are
-          // copied as is by GetLongPathNameW.  To also get the correct case
-          // for these parts, first convert to short file names and than back
-          // to long.
-          wchar_t buffer[32767] = L"";
-          int w_len = GetShortPathNameW (w_tmp.c_str (), buffer, 32767);
-          w_len = GetLongPathNameW (buffer, buffer, 32767);
-
-          if (! strip_marker)
-            retval = u8_from_wstring (std::wstring (buffer, w_len));
-          else if (w_len > 4)
-            retval = u8_from_wstring (std::wstring (buffer+4, w_len-4));
-
-          // If root is a drive, use an upper case letter for the drive letter.
-          if (retval.length () > 1 && retval[1] == ':')
-            retval[0] = toupper (retval[0]);
+          if (! w_tmp.empty ())
+            retval = u8_from_wstring (w_tmp);
         }
 #endif


Even with this change, the test suite executes still pretty slow.
According to "Very Sleepy CS" about 80% of the execution time is spent in "canonicalize_filename_mode" (I guess that is a gnulib module) effectively called by "find_private_fcn". Most of the time there is spent in "rpl_stat".

@jwe: In comment #49, you wrote that there is a different way to check for private functions. Is there anything I can do to help?

Markus Mützel <mmuetzel>
Group administrator
Sat 02 May 2020 02:56:12 PM UTC, comment #52: 

I tried with the attached patch that effectively removes all calls of "stat" in "find_private_fcn_file" on Windows.
I don't see a noticeable difference with or without it in the times reported by "time make check" on Linux.
However, running "__run_test_suite__" on Windows still is very slow.

I attached the profiler "Very Sleepy CS" [1] (which is a very handy tool btw) to Octave while it executed the test suite. It turns out that the biggest slow down seems to come from calling "canonicalize_file_name" frequently from load_path::package_info::find_private_fcn in load-path.cc. I guess we cannot remove "canonicalize_file_name" from that function, or can we?

Looking at the main contributors in "canonicalize_file_name", it turns out that especially the function calls that should lead to uniform letter casing on Windows have the longest execution time.
I added the "canonicalization" of the letter case (and conversion to long file names) to solve bug #56267:
http://hg.savannah.gnu.org/hgweb/octave/rev/def608acdfa9

Maybe we could make that part of "canonicalize_file_name" optional. Or probably better: we should use a different function where we need "canonical" letter case in file names and paths on Windows and remove that part of "canonicalize_file_name" altogether...
One possibility might be to internally store paths on Windows in lower case and only "prettify" the letter case close to the user interface (i.e. never for the use cases in load-path.cc). Mapping the strings to lower case is probably much faster than querying the correct casing from the file system.
I'm not sure what to do about short and long file names though. Maybe that won't be an issue here because we are looking for function files which should always be the long file name variant.
Any opinions?

[1]: http://www.codersnotes.com/sleepy/

(file #48985)

Markus Mützel <mmuetzel>
Group administrator
Tue 17 Mar 2020 03:32:44 PM UTC, comment #51: 

This is not so surprising.  stat() is a very Unixy sort of function, and probably has to be implemented atop various Windows kernel calls.  Going directly to Windows native libraries is definitely going to be faster.  This should probably be implemented if the stat() check is going to remain.  Although it sounds like jwe wants to get rid of it by a different architecture.

Rik <rik5>
Group administrator
Mon 16 Mar 2020 12:31:34 PM UTC, comment #50: 

Using "stat" to determine if a file exists is about 30 times slower than using "GetFileAttributesW" on Windows. That is 3000%!

I used the attached .oct file functions for the test. Running "file_exists" which loops 1e6 times took 5.4038 seconds. "file_stat_exists" which loops 1e4 times took 1.54844 seconds.

Even if this won't be the real fix. There is definitely room for performance improvement.

(file #48605, file #48606)

Markus Mützel <mmuetzel>
Group administrator
Thu 12 Mar 2020 07:11:23 PM UTC, comment #49: 

Yeah, access might just be implemented on top of stat.

In any case, the real fix is to not check for private functions the way I did in changeset 262cdfc6faf9.  It's not generally necessary to check for them that way, but it did fix the problem of reloading them after clear when the original directory was no longer in the path.  Instead, I think we need a better approach to function handles generally.  For me, this is a blocker for the release.

John W. Eaton <jwe>
Group administrator
Thu 12 Mar 2020 07:06:55 PM UTC, comment #48: 

Benchmarking shows that access() is about 8% slower than stat() on my machine.

Test code for stat() in stat_bm.cpp:


//#include <iostream>
#include <string>

#include <sys/types.h>
#include <sys/stat.h>
#include <unistd.h>

using namespace std;

int main()
{
   const int LOOPMAX = 1e7;
   const string path = "/home/rik/wip/Projects_Mine/octave-dev/afun.m";

   int retval;
   struct stat statbuf;
   for (int i = 0; i < LOOPMAX; i++)
     {
        retval = stat (path.c_str (), &statbuf);
//        cout << retval << "\n";
     }
}


Compiling and using "time ./stat_bm" shows an average running time of 11.8 (5 replicates).

Test code for access in access_bm.cpp:


//#include <iostream>
#include <string>
#include <unistd.h>

using namespace std;

int main()
{
   const int LOOPMAX = 1e7;
   const string path = "/home/rik/wip/Projects_Mine/octave-dev/afun.m";

   int retval;
   for (int i = 0; i < LOOPMAX; i++)
     {
        retval = access (path.c_str (), F_OK);
//        cout << retval << "\n";
     }
}


Same timing procedure yielded an average running time of 12.8.



(file #48586, file #48587)

Rik <rik5>
Group administrator
Thu 12 Mar 2020 06:40:37 PM UTC, comment #47: 

On Linux, if we don't need the rest of the information, we could try using access().  This checks file permissions, rather than grabbing all of the stat() data from the inode.


#include <unistd.h>

int access(const char *pathname, int mode);


Importantly, mode can be F_OK, which means just test for the existence of the file, rather than actually checking permissions.

We would want to benchmark it to make sure it was faster.

Rik <rik5>
Group administrator
Thu 12 Mar 2020 06:23:05 PM UTC, comment #46: 

If we are only interested in whether a file or folder of the given name exists, it might be more efficient on Windows to call GetFileAttributesW [1] and check the return value against INVALID_FILE_ATTRIBUTES.

Not sure what an equivalent function call on Linux would be. But if stat'ing is fast there, we could continue doing that.

[1]: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfileattributesw

Markus Mützel <mmuetzel>
Group administrator
Tue 10 Mar 2020 06:07:15 PM UTC, comment #45: 

I think the problem is not reloading but simply the addition of the following lines leads to a massive increase in the number of times that the stat function is called to check for a private function:


diff --git a/libinterp/corefcn/load-path.cc b/libinterp/corefcn/load-path.cc
--- a/libinterp/corefcn/load-path.cc
+++ b/libinterp/corefcn/load-path.cc
@@ -1675,6 +1716,8 @@
               retval = fname;
           }
       }
+    else
+      retval = find_private_fcn_file (canon_dir, fcn, type);

     return retval;
   }


I can think of other ways to deal with function handles that reference deleted functions, but I need to make sure that the solution we choose is compatible with Matlab behavior.  I'll try to come up with some tests and ask for help on the maintainers list.

This issue also brings up the question of whether there is a better way to check for the existence of a file other than calling stat.  The gnulib wrapper for that function seems to be particularly slow on Windows systems.  I many cases, we don't need all the information it returns.


John W. Eaton <jwe>
Group administrator
Sun 01 Mar 2020 07:32:52 PM UTC, comment #44: 

@jwe: profiling might give you a quick idea whether Octave is caught up re-loading files frequently.  I had the most luck using the 'perf' tool from the package linux-tools-common.  I would run it with


perf record -g -p XXX


where XXX was the process ID and the code was compiled with at least the '-g' option.

Rik <rik5>
Group administrator
Sun 01 Mar 2020 07:24:47 PM UTC, comment #43: 

As I mentioned I increased the test matrix size.
I attached the modified copy.

(file #48529)

Dmitri A. Sergatskov <dasergatskov>
Sun 01 Mar 2020 07:18:13 PM UTC, comment #42: 

Same tests on Mageia 7 64b on the same box:

>> t = cputime
>> __run__test_suite__
:
:
>> cputime - tt
ans = 143.40


and the io package loads in .6 seconds (loading all Java classes for all Java based spreadsheet interfaces, just like on Windows).

(Remarkable; until I think some years ago __run_test_suite__ used to be quite a bit faster on Windows than "make check" on Linux. Now that I ran __run_test_suite__ for the first time on Linux I see that Octave is much faster there, at least for file I/O)
(BTW I didn't know that to be able to run __run_test_suite__ , octave needs to be installed)

In the related maintainers ML thread where I first reported the performance degradation [1] I also mentioned that run.m would somehow show that it manipulates the load path.
Just a far-fetched throw in the dark (sorry, I don't know all about several Octave internals): does the fix for this bug report lead to extra manipulation of load paths? because (I suppose) fiddling load path stuff could also invoke PKG_ADD things and that could take a lot of time.

[1] https://octave.1599824.n4.nabble.com/Octave-on-Windows-takes-a-long-time-to-start-amp-𔙁-run-m-reloads-Of-packages-tt4695555.html

Philip Nienhuis <philipnienhuis>
Group Member
Sun 01 Mar 2020 07:01:19 PM UTC, comment #41: 

Dmitri, what benchmark script are you running?  The one posted as Octave2.m shows different sizes than the output you show in comment #37.

John W. Eaton <jwe>
Group administrator
Sun 01 Mar 2020 06:55:00 PM UTC, comment #40: 

Philip: No, you can skip that one.  Now I'm wondering whether this change is causing files to be reloaded unnecessarily or something like that because it doesn't seem to me that just computing the absolute file name should add so much to the execution time.

John W. Eaton <jwe>
Group administrator
Sun 01 Mar 2020 05:54:35 PM UTC, comment #39: 

@JWE:
You still want the results of the "stats-diffs.txt" patch (comment #31)?
I can at least make a start with that, although later tonight I need my box for work related things (preparing for a course tomorrow).

Philip Nienhuis <philipnienhuis>
Group Member
Sun 01 Mar 2020 05:49:01 PM UTC, comment #38: 

With the "cache-absolute-diffs.txt" patch (on top of rev. e82d00e8be37 "merge stable to default") I get the following (compare with comment #24):

>> t = cputime
>> __run__test_suite__
:
:
>> cputime - tt
ans = 961.59


and the io package load time, after several unload/load cycles (I read "cache") is about 12-13 seconds.
All of this is comparable to the "make-absolute-diffs.txt" patch of comment #22.

I really wonder why these change make Octave so slow on especially Windows, and if it's a local feature of my box (although at work I see about the same).
On Linux there isn't such a big performance degradation, AFAICS.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 29 Feb 2020 10:41:17 PM UTC, comment #37: 

Running benchmarks with the patch:

Octave 5.2.0

   III. Programmation
   ------------------
9,500,000 Fibonacci numbers calculation (vector calc)_ (sec): 1.5076
Creation of a 5250x5250 Hilbert matrix (matrix calc) (sec): 0.22328
Grand common divisors of 500,000 pairs (recursion)___ (sec): 0.13082
Creation of a 620x620 Toeplitz matrix (loops)_______ (sec): 3.4958
Escoufier's method on a 97x97 matrix (mixed)________ (sec): 5.1361
                  ------------------------------------------------------
                Trimmed geom. mean (2 extremes eliminated): 1.0558


908bdd05398a (before the patch):


   III. Programmation
   ------------------
9,500,000 Fibonacci numbers calculation (vector calc)_ (sec): 1.4632
Creation of a 5250x5250 Hilbert matrix (matrix calc) (sec): 0.405
Grand common divisors of 500,000 pairs (recursion)___ (sec): 0.13471
Creation of a 620x620 Toeplitz matrix (loops)_______ (sec): 5.978
Escoufier's method on a 97x97 matrix (mixed)________ (sec): 8.4816
                  ------------------------------------------------------
                Trimmed geom. mean (2 extremes eliminated): 1.5244


908bdd05398a+ (after the patch):


   III. Programmation
   ------------------
9,500,000 Fibonacci numbers calculation (vector calc)_ (sec): 1.5852
Creation of a 5250x5250 Hilbert matrix (matrix calc) (sec): 0.43049
Grand common divisors of 500,000 pairs (recursion)___ (sec): 0.14574
Creation of a 620x620 Toeplitz matrix (loops)_______ (sec): 4.8253
Escoufier's method on a 97x97 matrix (mixed)________ (sec): 6.7766
                  ------------------------------------------------------
                Trimmed geom. mean (2 extremes eliminated): 1.4877


FWIW

Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Sat 29 Feb 2020 10:07:39 PM UTC, comment #36: 

Thanks, John.
Hopefully tomorrow night I can give it a try (busy weekend), otherwise next Monday.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 29 Feb 2020 09:35:08 PM UTC, comment #35: 

Or, can you try this one instead?  It removes most of the calls to canonicalize_file_name and replaces the rest with make_absolute.  I think the end result should be performance that is close to what we had before I tried to fix this bug.  If not, then please let me know how bad it is for you and I'll keep trying.

(file #48527)

John W. Eaton <jwe>
Group administrator
Sat 29 Feb 2020 09:19:23 PM UTC, comment #34: 

The patch from comment #31 should be applied instead of the one from comment #22.

John W. Eaton <jwe>
Group administrator
Sat 29 Feb 2020 04:51:14 PM UTC, comment #33: 

To what revision should stats-diff.txt be applied?
I've already applied the previous patch from comment #22, should that be backed out first?

Philip Nienhuis <philipnienhuis>
Group Member
Fri 28 Feb 2020 09:33:46 PM UTC, comment #32: 

@JWE:
comment #31:
Sure, I'll gladly try, but I have a some private things this weekend. Let's see how far I get.

comment #27:
Yes I do see slower performance, but it's hard to pinpoint exactly what & when.
Most of my work at the office is on network drives, but with drive letters; I have no idea if those drive letter paths are silently remorphed into UNC paths or so, nor if that even makes a difference. The network is usually fairly fast unless really big files (~100s of MB) need to be processed. That suggests that network resources are somehow cached.

Just as a reminder:
I first hit this when watching the io package being loaded on Windows. A routine _init_io_.m calls a routine that in turn checks the javaclasspath, loads Java class libs for spreadsheet I/O and then searches for LibreOffice and loads LO's Java class libs. That second routine is called twice, first for "regular" Java class libs, then for LO's. All of this only concerns local files and directories: one flat directory in my user profile and LibreOffice\ in 'C:\Program Files\'.
The execution time of all this used to be 2-3 seconds, with your first canonicalize_file_name patch up to ~30 seconds, with your latest make_absolute_filename patch ~12 seconds.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 28 Feb 2020 08:07:03 PM UTC, comment #31: 

I made the attached change to see just how many times the make_absolute (or canonicalize_file_name) functions were called when running the test suite.  My results are that just starting Octave shows

+verbatim-
mk_abs_fcn_info = 0
mk_abs_symscope = 33
mk_abs_load_path_pi_remove = 0
mk_abs_load_path_pi_find_private_fcn = 640
mk_abs_load_path_pi_find_private_fcn_already_absolute = 640
mk_abs_load_path_pi_add_to_private_fcn_map = 126
mk_abs_load_path_pi_add_to_method_map = 0
mk_abs_load_path_pi_remoave_private_fcn_map = 0
+verbatim-

and after _run_test_suite_ I see

+verbatim-
mk_abs_fcn_info = 97
mk_abs_symscope = 41247
mk_abs_load_path_pi_remove = 22
mk_abs_load_path_pi_find_private_fcn = 6125137
mk_abs_load_path_pi_find_private_fcn_already_absolute = 6125137
mk_abs_load_path_pi_add_to_private_fcn_map = 29662
mk_abs_load_path_pi_add_to_method_map = 4
mk_abs_load_path_pi_remoave_private_fcn_map = 22
-verbatim-

So the vast majority of calls are in load_path::package_info::find_private_fcn and none appear to be needed (!).  I added the call to check that when I noticed that most calls to make_absolute were happening in this one location.

Even though make_absolute calls absolute_pathname and exits early if the name is already absolute, it seems to slow things down significantly just to make the call.

Philip, can you try the attached patch and confirm my results on Windows?

If I can convince myself that find_private_fcn will always be called with an absolute directory name, then we don't need to call make_absolute there and it should speed things up significantly.

(file #48520)

John W. Eaton <jwe>
Group administrator
Fri 28 Feb 2020 07:52:31 PM UTC, comment #30: 

The benchmark code snippet (for comment 17):


% (4)
cumulate = 0; b = 0;
for i = 1:runs
  b = zeros(620, 620);
  tic;
    for j = 1:620
      for k = 1:620
        b(k,j) = abs(j - k) + 1;
      end
    end
  timing = toc;
  cumulate = cumulate + timing;
end
timing = cumulate/runs;
times(4, 3) = timing;
disp(['Creation of a 620x620 Toeplitz matrix (loops)_______ (sec): ' num2str(timing)])
clear b; clear j; clear k;


The original sciviews benchmark was released under GPL 2.
(I attached a copy for the reference).
I use it with significantly larger matrix size.

Dmitri.
--


(file #48518, file #48519)

Dmitri A. Sergatskov <dasergatskov>
Fri 28 Feb 2020 07:39:26 PM UTC, comment #29: 

WRT comment 18 -- no I did not try bisecting.
the hg I referenced was just last saved benchmark.
(I run them periodically, but not every time).


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Fri 28 Feb 2020 06:00:03 PM UTC, comment #28: 

RE comment #25: If you have installed Octave, then _run_test_suite_ should also work on Linux systems.  If not, then you should be able to run make check to run the tests in your build tree.

John W. Eaton <jwe>
Group administrator
Fri 28 Feb 2020 05:58:09 PM UTC, comment #27: 

Running the tests on my Debian system also still works, but it is a little slower after the changeset that I checked in to fix this bug.

I agree that the test suite performance is not critical.  A change like the one I made might cause a significant change in performance for the tests because they parse most if not nearly all .m files.  Another big time sink in the test is running makeinfo to generate usage messages.

The real question is whether the change I made affects real usage.  Philip, are you noticing a general problem with performance, or is it just the test suite?  If it is a general problem, can you provide some example code that demonstrates the performance issue?  Also, are there directories in your Octave load-path that are on network drives?  I think that could be a significant problem, at least with the patch that uses canonicalize_file_name.

John W. Eaton <jwe>
Group administrator
Fri 28 Feb 2020 04:42:47 PM UTC, comment #26: 

The BIST tests within each m-file and the fixed tests in the test/ directory are designed to check Octave's correctness of behavior.  They aren't designed as a benchmark for testing performance.  I have argued that we we need a performance benchmark so that we don't unintentionally introduce regressions in performance, as seems to have happened in this report.  However, with limited developer time we just haven't gotten around to it.  I just added this project under the Performance category in the Octave Wiki.


* Develop a performance benchmark for Octave (interpreter, load/save, plotting, etc., but not simply tests of underlying libraries such as BLAS or LAPACK).  This benchmark could be run periodically to make sure that changes during development do not introduce regressions in performance.


Second, _run_test_suite_ works fine for me on Linux.

Rik <rik5>
Group administrator
Fri 28 Feb 2020 11:32:46 AM UTC, comment #25: 

Just FTR, I think __run_test_suite__.m may not be a very representative but it checks all Octave functions so does tell something about general Octave performance.

Is there a similar command on Linux? I found that __run_test_suite__.m doesn't work there.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 27 Feb 2020 09:58:59 PM UTC, comment #24: 

(First two verbatim blocks copied from comment #15)
Patch of comment #7 backed out:

>> t = cputime
>> __run__test_suite__
:
:
>> cputime - tt
ans = 281.88


with patch of comment #7:

>> t = cputime
>> __run__test_suite__
:
:
>> cputime - tt
ans = 1914.2


OK.
Now with new patch of comment #22:

>> t = cputime
>> __run__test_suite__
:
:
>> cputime - tt
ans = 964.60


(all on the same PC)
So execution time is now ~50 % reduced, but still about 3.5 times slower than before the first fix.

To me this sill feels like a big sacrifice for fixing the actual bug here.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 26 Feb 2020 07:54:01 PM UTC, comment #23: 

Thanks, John.

I hoped someone could confirm my findings on Windows with

__run_test_suite__.m nomarkup-. Octave felt much slower but I didn't expect it to be *that* much slower.

Is the new cset to be applied "on top of" the one in comment #7? I guess so, looking at the cset.
I'll try the cset tomorrow.

Philip Nienhuis <philipnienhuis>
Group Member
Wed 26 Feb 2020 06:58:34 PM UTC, comment #22: 

I also see the test suite taking longer on my Debian system but it is not as bad as Philip reports for Windows.

I see now that canonicalize_file_name has to do a lot of work each time to check for symbolic links.  I wasn't thinking about that when I made the change.  I think we do need to store the full file name so we can correctly reload those handles to private functions.  But maybe we can use the absolute name instead of the canonical name?  That should be more efficient.  Maybe we can also reduce the number of calls to make the directory or file name absolute?  Does the attached change help?

(file #48502)

John W. Eaton <jwe>
Group administrator
Sun 23 Feb 2020 04:22:13 PM UTC, comment #21: 

@Markus:
Could you confirm my observation with __run_test_suite__.m with and without patch backed out?

Philip Nienhuis <philipnienhuis>
Group Member
Sun 23 Feb 2020 11:55:37 AM UTC, comment #20: 

@Philip: With that changeset, "canonicalize_file_name" is called a lot more often behind the scenes. Still not sure if this is causing the slowdown though.

Marking as a blocker for the 6.1 release because the impact on performance is quite dramatic.

Markus Mützel <mmuetzel>
Group administrator
Fri 21 Feb 2020 07:47:07 PM UTC, comment #19: 

@Markus:
(sorry for a late response, had to scan all PCs here after our provider limited internet access upon detecting "Clop" ransomware traffic from my daughters Macbook)

The most prominent difference is really after backing out JWE's changeset (comment #7, 262cdfc6faf9) and then running __run_test_suite__ .
That indicates that it isn't quite canonicalize_file_name that is affected, it seems many many more functions show a slowdown.
It's intriguing that it is mostly Windows that is affected - that is, I had no chance yet to compare the effects of that cset on Linux.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 21 Feb 2020 03:28:30 PM UTC, comment #18: 

@Dmitri: I don't see how hg id 8ac3222bf951 or the following change could affect the performance of toeplitz. Did you find that changeset by bisecting?

Markus Mützel <mmuetzel>
Group administrator
Tue 18 Feb 2020 04:41:12 PM UTC, comment #17: 

I ran (sciviews) benchmarks and I see a drastic increase in
Toeplitz matrix creation (loops) (2.8 sec --> 7.9 sec) since
hg_id 8ac3222bf951 (2019-11-28).

Dmitri.


Dmitri A. Sergatskov <dasergatskov>
Tue 18 Feb 2020 03:58:01 PM UTC, comment #16: 

I can confirm that running the test suite is much slower lately. But I couldn't pinpoint it to any specific change yet.

Is calling "canonicalize_file_name" quite frequently causing the slowdown?

Markus Mützel <mmuetzel>
Group administrator
Mon 17 Feb 2020 10:06:15 PM UTC, comment #15: 

I found that simply running __run_test_suite__.m in a Windows installation clearly shows an enormous performance hit.

With the patch of comment #7 (262cdfc6faf9):

>> t = cputime
>> __run__test_suite__
:
:
>> cputime - tt
ans = 1914.2


Same crossbuild, with the patch backed-out:

>> t = cputime
>> __run__test_suite__
:
:
>> cputime - tt
ans = 281.88


My crossbuilds have some additional patches but they're the same in the above comparison. I did unload all OF packages before the tests.

Can someone with a windows box confirm?

Philip Nienhuis <philipnienhuis>
Group Member
Tue 11 Feb 2020 08:28:10 PM UTC, comment #14: 

Changeset 28026:262cdfc6faf9 (comment #7) affects some performance on Windows boxes, see:
https://octave.1599824.n4.nabble.com/Octave-on-Windows-takes-a-long-time-to-start-amp-𔙁-run-m-reloads-Of-packages-tt4695555.html

Philip Nienhuis <philipnienhuis>
Group Member
Sun 09 Feb 2020 11:26:18 AM UTC, comment #13: 

I'm not sure if supporting UNC paths alters the definition of canonicalize_file_name. I can't find a reference to the fact that it only works for local files on its man page. [1]

Also, the std::filesystem library (C++17) adds std::filesystem::canonical [2]. Reading on (e.g. the definition of "root-name" here [3]), I'd understand that it works on UNC paths, too.
Maybe, we could replace gnulib's canonicalize_file_name with the standard function std::filesystem::canonical at some point in the future. With respect to UNC paths, that wouldn't lead to much change in behavior now.

I agree that the special case I introduced in my last change could be improved. But I hope it is good enough until when we allow using C++17 functions (if we decide to do that).

Alternatively, we could introduce a new function that behaves more along the lines of std::filesystem::canonical. But I think the differences would be so small that it would only cause confusion.

[1]: http://man7.org/linux/man-pages/man3/canonicalize_file_name.3.html
[2]: https://en.cppreference.com/w/cpp/filesystem/canonical
[3]: https://en.cppreference.com/w/cpp/filesystem/path

Markus Mützel <mmuetzel>
Group administrator
Sat 08 Feb 2020 08:17:18 PM UTC, comment #12: 

So this is the opposite of what I suggested in bug #57664.

Do we really want to alter the definition of canonicalize_file_name in this manner? I don't use Windows so it doesn't affect me, but it seems really counter to the entire stated purpose of this function.

Mike Miller <mtmiller>
Group Member
Sat 08 Feb 2020 12:58:13 PM UTC, comment #11: 

I pushed a changeset here to make canonicalize_file_name work with UNC shares on Windows:
http://hg.savannah.gnu.org/hgweb/octave/rev/087a21522aa3

This should also solve the issue here.

Markus Mützel <mmuetzel>
Group administrator
Sat 08 Feb 2020 07:24:41 AM UTC, comment #10: 

Calling private functions in UNC shares seems to be broken indeed:

>> cd('\\SERVER\share\test')
>> addpath('a')
>> type a.m
a.m is the user-defined function defined from: \\SERVER\share\test\a\a.m

function a()

c;

end

>> type private/c.m
private/c.m is the user-defined function defined from: \\SERVER\share\test\a\private/c.m

function c()

disp('c in a');

end

>> a
error: 'c' undefined near line 3, column 3
error: called from
    a at line 3 column 1


Re-opening this bug for now. Or would it be better to open a new one for this regression?

Markus Mützel <mmuetzel>
Group administrator
Tue 04 Feb 2020 07:43:42 AM UTC, comment #9: 

Given that canonicalize_file_name fails for Windows UNC paths (returns empty string), I'm not sure if the canonicalized path can be safely used as the key for the private functions.
I haven't tried yet. But we should probably revisit this change.
I'll report when I had a chance to testing private functions on UNC paths.

Markus Mützel <mmuetzel>
Group administrator
Mon 03 Feb 2020 09:53:05 PM UTC, comment #8: 

Okay to close now?

Rik <rik5>
Group administrator
Thu 30 Jan 2020 08:37:35 PM UTC, comment #7: 

Thanks for checking.  I pushed the following changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/262cdfc6faf9

I see this as a crude patch to get past this particular problem and still believe that a more complete refactoring of the load path and function objects in the interpreter is needed.  Maybe that can happen for version 7.

John W. Eaton <jwe>
Group administrator
Thu 30 Jan 2020 07:49:25 PM UTC, comment #6: 

With your changes from comment #5, the tests for publish.tst no longer fail when I run the test suite.

Markus Mützel <mmuetzel>
Group administrator
Thu 30 Jan 2020 04:36:28 PM UTC, comment #5: 

I spent a bunch more time looking at this and finally came up with the attached patch.

We probably also need similar changes to use absolute/canonical directory names more completely in the load path.  And possibly to completely overhaul the load path classes.  And rethink the way that function objects are stored in the octave_value class hierarchy.  But maybe this change is enough for now.

Can you try it and see if it works for you?  Meanwhile, I will write a commit message and get it ready to push.

(file #48306)

John W. Eaton <jwe>
Group administrator
Thu 30 Jan 2020 08:14:42 AM UTC, comment #4: 

Imho, we should try to allow calling (at least) Octave core functions in any given order. If that avoids the error, we should move the files.

Maybe we should also check if other functions (at least in Octave core) that are "mlock"ed use function handles to private or local(?) functions and evaluate what should be done.

The closest we have to a "Release Notes" is probably the "NEWS" file. Maybe we could add a "Known issues/regressions" section.

Markus Mützel <mmuetzel>
Group administrator
Wed 29 Jan 2020 06:57:02 PM UTC, comment #3: 

I can confirm that moving these three functions up one level from image/private avoids the problem in the test suite and have a changeset ready to push if we decide to make this change for the release.

I'm not sure where to document this problem other than here.

John W. Eaton <jwe>
Group administrator
Wed 29 Jan 2020 03:17:55 PM UTC, comment #2: 

Are the only functions that cause the error for "publish.m" these?

  • __imfinfo__.m
  • __imread__.m
  • __imwrite__.m


As a work-around for Octave 6, could we move these files from the "private" folder to scripts/image? Their names are surrounded by "__" anyway, marking them as internal functions.
Once this bug is actually fixed, we could move them back and make them private again.

We should probably also document somewhere that this will be a known limitation in Octave 6.

Markus Mützel <mmuetzel>
Group administrator
Wed 29 Jan 2020 12:14:22 AM UTC, comment #1: 

I think we have some fairly deep bugs related to the way functions and function handles are managed in the interpreter and octave_value object hierarchy.  I don't see an easy fix for them and so I think we will probably have to wait until after the 6.1 release to fix them.

The problem with the publish/publish.tst test failure requires the following to fail:

  • execute imformats
  • clear all (or clear functions) so that the handles to the private functions that are defined in imformats are no longer valid
  • attempt to use publish


If the call to mlock is removed in imformats, the above sequence will work, but then clearing the imformats function will lose any new formats that have been added by a user or a PKG_ADD file so that's probably not a good fix for this problem and is obviously not a fix for the real problem (function handles don't properly reload the functions they refer to if those functions are cleared).

John W. Eaton <jwe>
Group administrator
Wed 18 Dec 2019 06:06:20 PM UTC, original submission:  

Bug #57424 noted a failure in the publish.tst test files in the test suite after a recent change.

This problem appears to be due to a combination of factors, including the use of handles to private functions in imformats, a call to "clear functions" in test/bug-35881/bug35881.m, and possibly the use of mlock in imformats that prevents the function handles from being created again when needed (though recreating the handles really shouldn't be necessary - they should remain valid even after "clear functions" is executed).

A simpler test case is attached.  After unpacking the files, execute the following commands:


cd fh-bug
fh1 = f1()
fh2 = f2()
fh1 ()
fh2 ()
functions (fh1)
functions (fh2)
cd subdir
fh1 ()
fh2 ()
functions (fh1)
functions (fh2)
clear functions
fh1 ()
fh2 ()
functions (fh1)
functions (fh2)


There should be no error, but Octave throws an error about invalid function handles after moving to subdir or after the "clear functions" command (if the cd command is skipped).

There is also some discussion about this problem in the following thread on the maintainers list:

https://lists.gnu.org/archive/html/octave-maintainers/2019-12/msg00108.html

John W. Eaton <jwe>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #48985:  bug57439_avoid_stat.patch added by mmuetzel (12KiB - application/octet-stream)
file #48605:  file_exists.cc added by mmuetzel (2KiB - application/octet-stream)
file #48606:  file_stat_exists.cc added by mmuetzel (1KiB - application/octet-stream)
file #48586:  stat_bm.cpp added by rik5 (420B - text/x-c++src)
file #48587:  access_bm.cpp added by rik5 (348B - text/x-c++src)
file #48529:  Octave2l.m added by dasergatskov (9KiB - text/x-objcsrc)
file #48527:  cache-absolute-diffs.txt added by jwe (5KiB - text/plain)
file #48520:  stats-diffs.txt added by jwe (6KiB - text/plain)
file #48518:  Octave2.m added by dasergatskov (9KiB - text/x-objcsrc)
file #48519:  gcd2.m added by dasergatskov (302B - text/x-objcsrc)
file #48502:  make-absolute-diffs.txt added by jwe (3KiB - text/plain)
file #48306:  private-fcn-diffs.txt added by jwe (13KiB - text/plain)
file #48086:  fh-bug.tar.gz added by jwe (335B - application/gzip)

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

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

    Date Changed by Updated Field Previous Value => Replaced by
    2020-06-23 mmuetzel StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2020-05-02 mmuetzel Attached File- Added bug57439_avoid_stat.patch, #48985
    2020-03-16 mmuetzel Attached File- Added file_exists.cc, #48605
        Attached File- Added file_stat_exists.cc, #48606
    2020-03-12 rik5 Attached File- Added stat_bm.cpp, #48586
        Attached File- Added access_bm.cpp, #48587
    2020-03-01 dasergatskov Attached File- Added Octave2l.m, #48529
    2020-02-29 jwe Attached File- Added cache-absolute-diffs.txt, #48527
    2020-02-28 jwe Attached File- Added stats-diffs.txt, #48520
    2020-02-28 dasergatskov Attached File- Added Octave2.m, #48518
        Attached File- Added gcd2.m, #48519
    2020-02-26 jwe Attached File- Added make-absolute-diffs.txt, #48502
    2020-02-23 mmuetzel Severity3 - Normal 5 - Blocker
        Item GroupMatlab Compatibility Regression
        StatusReady For Test Confirmed
        Releasedev 6.0.90
    2020-02-17 philipnienhuis Carbon-Copy- Added philipnienhuis
    2020-02-08 mmuetzel StatusNone Ready For Test
    2020-02-08 mmuetzel StatusFixed None
        Open/ClosedClosed Open
    2020-02-03 jwe StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2020-01-30 jwe StatusPatch Submitted Ready For Test
    2020-01-30 jwe StatusNone Patch Submitted

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code