bugGNU Octave - Bugs: bug #50105, __ilu__ and __ichol__ should use...

 
 

bug #50105: __ilu__ and __ichol__ should use liboctave functions instead of feval

Submitter:  John W. Eaton <jwe>
Submitted:  Fri 20 Jan 2017 07:05:46 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Performance
Status:  Fixed Assigned to:  siko1056
Originator Name:  jwe Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 10 Jan 2019 02:38:45 AM UTC, comment #8: 

Never mind, I'm totally wrong:

https://stackoverflow.com/questions/584824/guaranteed-lifetime-of-temporary-in-c

I thought this was a problem because someone said they had come up with a test case that showed a problem with code similar to this.

Jordi GutiƩrrez Hermoso <jordigh>
Group Member
Wed 09 Jan 2019 11:41:05 PM UTC, comment #7: 

Regarding comment #4 and for curiosity, which bug was introduced, or how did you notice this problem?

Kai Torben Ohlhus <siko1056>
Group Member
Wed 09 Jan 2019 10:57:11 PM UTC, comment #6: 

Maybe this is too deep questions of the compiler, but isn't the pointer in the original code valid throughout the parsing of the function call to ichol_t?

In pseudo-code, how does the compiler handle this?


fcn_call (A, B, fcn_call2 (A2, B2).fortran_vec (), C);
line2;


If fcn_call2 returns a Matrix object for which we then call fortran_vec() I don't think the compiler arranges to call the destructor until line2 is reached.  Since we don't need the results beyond the execution of fcn_call I believe this is okay.

Rik <rik5>
Group administrator
Wed 09 Jan 2019 10:28:03 PM UTC, comment #5: 

The data in cols_norm should only be needed until the end of the block, correct?  If so, then this change seems fine to me.

John W. Eaton <jwe>
Group administrator
Wed 09 Jan 2019 10:07:59 PM UTC, comment #4: 

I just noticed that this introduced a bug.

I think this should fix it:


     {
       SparseMatrix L;
       SparseMatrix sm_l = Ftril (args(0))(0).sparse_matrix_value ();
+      Array <Complex> cols_norm = xcolnorms (sm_l, 1);
       ichol_t <SparseMatrix,
                double, ichol_mult_real, ichol_checkpivot_real>
-               (sm_l, L, xcolnorms (sm_l, 1).fortran_vec (), droptol, michol);
+               (sm_l, L, cols_norm.fortran_vec (), droptol, michol);

       return ovl (L);
     }


The issue is that the function call to xcolnorms returns a matrix object, which after getting its fortran_vec goes out of scope, so the pointer now points to memory that has been freed by the dtor of the intermediate xcolnorms matrix.

Jordi GutiƩrrez Hermoso <jordigh>
Group Member
Fri 27 Jan 2017 03:41:55 PM UTC, comment #3: 
Kai Torben Ohlhus <siko1056>
Group Member
Sun 22 Jan 2017 09:44:09 PM UTC, comment #2: 

As co-author of the functions, I take care of this problem and apply a patch to default in a few days. It does not cause problems, but might shorten the running time, so I don't assume stable to be the right place.

Kai Torben Ohlhus <siko1056>
Group Member
Sat 21 Jan 2017 11:09:12 PM UTC, comment #1: 

Amen to that.  For reference, here are the function invocations in question.


corefcn/__ichol__.cc:201:      sm = octave::feval ("tril", arg_list)(0).sparse_matrix_value ();
corefcn/__ichol__.cc:211:      sm = octave::feval ("tril", arg_list)(0).sparse_complex_matrix_value ();
corefcn/__ichol__.cc:444:        octave::feval ("tril", arg_list)(0).sparse_matrix_value ();
corefcn/__ichol__.cc:448:      cols_norm = octave::feval ("norm", arg_list)(0).vector_value ();
corefcn/__ichol__.cc:462:        octave::feval ("tril", arg_list)(0).sparse_complex_matrix_value ();
corefcn/__ichol__.cc:466:      cols_norm = octave::feval ("norm", arg_list)(0).complex_vector_value ();
corefcn/__ilu__.cc:163:      retval(1) = octave::feval ("triu", arg_list)(0).sparse_matrix_value ();
corefcn/__ilu__.cc:165:        octave::feval ("speye", ovl (sm.cols ()))(0).sparse_matrix_value ();
corefcn/__ilu__.cc:168:        octave::feval ("tril", arg_list)(0).sparse_matrix_value ();
corefcn/__ilu__.cc:176:      retval(1) = octave::feval ("triu", arg_list)(0).sparse_complex_matrix_value ();
corefcn/__ilu__.cc:178:        octave::feval ("speye", ovl (sm.cols ()))(0).sparse_complex_matrix_value ();
corefcn/__ilu__.cc:181:        octave::feval ("tril", arg_list)(0).sparse_complex_matrix_value ();
corefcn/__ilu__.cc:497:      SparseMatrix sm_u = octave::feval ("triu", arg_list)(0).sparse_matrix_value ();
corefcn/__ilu__.cc:499:      SparseMatrix sm_l = octave::feval ("tril", arg_list)(0).sparse_matrix_value ();
corefcn/__ilu__.cc:501:      rows_norm = octave::feval ("norm", arg_list)(0).vector_value ();
corefcn/__ilu__.cc:503:      cols_norm = octave::feval ("norm", arg_list)(0).vector_value ();
corefcn/__ilu__.cc:511:      SparseMatrix eye = octave::feval ("speye", arg_list)(0).sparse_matrix_value ();
corefcn/__ilu__.cc:519:        octave::feval ("triu", arg_list)(0).sparse_complex_matrix_value ();
corefcn/__ilu__.cc:522:        octave::feval ("tril", arg_list)(0).sparse_complex_matrix_value ();
corefcn/__ilu__.cc:524:      rows_norm = octave::feval ("norm", arg_list)(0).complex_vector_value ();
corefcn/__ilu__.cc:526:      cols_norm = octave::feval ("norm", arg_list)(0).complex_vector_value ();
corefcn/__ilu__.cc:535:        octave::feval ("speye", arg_list)(0).sparse_complex_matrix_value ();
corefcn/__ilu__.cc:969:      nnz_u = (octave::feval ("triu", arg_list)(0).sparse_matrix_value ()).nnz ();
corefcn/__ilu__.cc:971:      nnz_l = (octave::feval ("tril", arg_list)(0).sparse_matrix_value ()).nnz ();
corefcn/__ilu__.cc:976:      rc_norm = octave::feval ("norm", arg_list)(0).vector_value ();
corefcn/__ilu__.cc:985:        octave::feval ("speye", arg_list)(0).sparse_matrix_value ();
corefcn/__ilu__.cc:1018:        octave::feval ("triu", arg_list)(0).sparse_complex_matrix_value ().nnz ();
corefcn/__ilu__.cc:1021:        octave::feval ("tril", arg_list)(0).sparse_complex_matrix_value ().nnz ();
corefcn/__ilu__.cc:1026:      rc_norm = octave::feval ("norm", arg_list)(0).complex_vector_value ();
corefcn/__ilu__.cc:1036:        octave::feval ("speye", arg_list)(0).sparse_complex_matrix_value ();


Also, see bug #50116 where feval should be replaced with direct calls to C++ functions in libinterp using the Fname_of_function syntax.

Rik <rik5>
Group administrator
Fri 20 Jan 2017 07:05:46 PM UTC, original submission:  

In _ilu_.cc and _ichol_.cc, there are many calls to feval to execute the functions tril, triu, speye, and norm that should use liboctave functions directly.

Using feval for this is inefficient since it may involve the interpreter and always involves unnecessary conversion to/from octave_value objects.

For norm, we already have functions in liboctave to do the job.

For speye, I think it should be possible to call the sparse matrix contructor to generate a sparse identity matrix.  If not, then we could easily define an speye function in liboctave.

For tril and triu, we should probably move the templates from corefcn/tril.cc to liboctave and export them in a public header file.

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:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by siko1056 (Posted a comment)
  • -email is unavailable- added by rik5 (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-01-27 siko1056 StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2017-01-22 siko1056 StatusConfirmed In Progress
        Assigned toNone siko1056
    2017-01-21 rik5 StatusNone Confirmed
        Operating SystemBSD Any

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code