bugGNU Octave - Bugs: bug #59192, unwind_protect objects should be...

 
 

bug #59192: unwind_protect objects should be replaced with lighter weight alternatives

Submitter:  Markus Mützel <mmuetzel>
Submitted:  Sun 27 Sep 2020 03:23:17 PM UTC
   
 
Category:  Interpreter Severity:  1 - Wish
Priority:  3 - Low Item Group:  None
Status:  Fixed Assigned to:  None
Originator Name:  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

Wed 30 Sep 2020 09:43:43 PM UTC, comment #21: 

Good, but it doesn't need to be a high priority.  I think I saw only two instances where having a variadic unwind_protect_var could be used.

Rik <rik5>
Group administrator
Wed 30 Sep 2020 09:24:24 PM UTC, comment #20: 

Thanks for working on these changes.  I'll see if I can create an implementation of the templates I suggested in comment #8.

John W. Eaton <jwe>
Group administrator
Wed 30 Sep 2020 08:15:04 PM UTC, comment #19: 

I checked in several other changesets adapting as many unwind_protect objects as I could.  At this point the list of remaining instances to review is in the uploaded file uprotect4.list.  None of the remaining code pieces will be straightforward to fix without more extensive re-coding or additional functionality being added to the unwind_protect_var class.  Hence, I think this bug can stop here.  Any further development can be its own project by an interested coder or a new bug report.  Marking as fixed and closing report.

(file #49896)

Rik <rik5>
Group administrator
Tue 29 Sep 2020 02:17:03 PM UTC, comment #18: 

I fixed three more small easy instances here: http://hg.savannah.gnu.org/hgweb/octave/rev/6ec04c206ac4.

I've uploaded the revised list of instances to fix as uprotect2.list.

(file #49885)

Rik <rik5>
Group administrator
Tue 29 Sep 2020 02:02:35 PM UTC, comment #17: 

I like the readability of "restore_var".  I found a simple pattern that a few of the files had for protecting call_depth and made those changes here: http://hg.savannah.gnu.org/hgweb/octave/rev/aef839f40da2.


Rik <rik5>
Group administrator
Tue 29 Sep 2020 01:58:18 PM UTC, comment #16: 

I've just been using "upv" and "act" for these variable names, but something like restore_var is probably better.  Whatever we decide, it would probably be best to have some consistency.

John W. Eaton <jwe>
Group administrator
Tue 29 Sep 2020 01:40:35 PM UTC, comment #15: 

I made the changes that Rik suggested and pushed here:
https://hg.savannah.gnu.org/hgweb/octave/rev/0eb855b7798b

Markus Mützel <mmuetzel>
Group administrator
Mon 28 Sep 2020 11:17:35 PM UTC, comment #14: 

Thanks for tackling graphics.cc; there are 19 instances in that file.  I tried the patch and it doesn't seem to cause any problems for me (I ran the tests and demos in the plot directory).

One thought, in a replacement like this


-  octave::unwind_protect frame;
-  frame.protect_var (updating_axes_layout);
-  updating_axes_layout = true;
+  octave::unwind_protect_var<bool>
+    restore_updating_axes_layout (updating_axes_layout, true);


would it be simpler to call the instance "restore_var"?  The code would then become


+  octave::unwind_protect_var<bool>
+    restore_var (updating_axes_layout, true);


which is shorter and it is clear which variable is being saved because one can read across in English and see "restore variable updating_axes_layout".


Rik <rik5>
Group administrator
Mon 28 Sep 2020 06:44:42 PM UTC, comment #13: 
Markus Mützel <mmuetzel>
Group administrator
Mon 28 Sep 2020 06:35:42 PM UTC, comment #12: 

Right, interpreter_try is another example of what I was talking about in comment #5.

John W. Eaton <jwe>
Group administrator
Mon 28 Sep 2020 06:24:50 PM UTC, comment #11: 

I tried to take a stab at graphics.cc. See the attached patch.

There are two places where unwind_protect is used where I don't know how to replace it easily:
https://hg.savannah.gnu.org/hgweb/octave/file/d28016d16e9a/libinterp/corefcn/graphics.cc#l3727

      octave::unwind_protect frame;

      interpreter_try (frame);


And Fdrawnow which seems to be a real life example of what jwe wrote in comment #10:
https://hg.savannah.gnu.org/hgweb/octave/file/d28016d16e9a/libinterp/corefcn/graphics.cc#l13837


(file #49878)

Markus Mützel <mmuetzel>
Group administrator
Mon 28 Sep 2020 06:14:53 PM UTC, comment #10: 

Rik: Yeah, me too.  But there are things like this (just an example, not real code):


unwind_protect frame;

frame.protect_var (v1);
v1 = some_value;

frame.add_fcn (some_fcn, ...);

frame.protect_var (v2);
v2 = some_other_value;

if (some_condition)
  {
     frame.add_fcn (another_fcn, ...);

     do_some_stuff ();
  }


I could guess but wouldn't know for sure without doing some careful checking whether it would be OK to rearrange the order in which things are saved/restored (actual code might be much more complex than the bit above).  In some cases it may be OK to create a new unwind_action inside an inner scope (see the use of another_fcn above) and others adding it to the unwind_protect object is simply conditional and the action needs to execute at the end of the outer scope.  So we might need to be able to write something like this:


unwind_action act;

if (condition)
  act = unwind_action (...);

// OR (perhaps to avoid confusion?)

if (condition)
  act.replace_action (...);


Copying unwind_action objects is currently not allowed, probably because doing so could cause some confusion.

John W. Eaton <jwe>
Group administrator
Mon 28 Sep 2020 05:44:07 PM UTC, comment #9: 

Rats!  I was hoping changes would not involve a lot of judgment.

In any case, I like the idea of variadic unwind_protect statements for variables.

Rik <rik5>
Group administrator
Mon 28 Sep 2020 04:25:08 PM UTC, comment #8: 

I started looking at replacing calls to frame.protect_var with unwind_protect_var<T> objects because I thought that it might be possible to do most of them fairly easily.  But I quickly discovered (after looking at just one file!) that there are likely to be many cases that will need some thought.

I still consider this to be a good change to make but would recommend doing this job carefully over time rather than trying to do it quickly or semi-automatically.

It might also be nice to have variadic templates that would allow something like


unwind_protect_vars<T1, T2, ...> upv1 (var1, var2, ...);

unwind_protect_vars_with_vals<T1, T2, ...>
  upv2 (var1, new_val1, var2, new_val2, ...);


John W. Eaton <jwe>
Group administrator
Mon 28 Sep 2020 02:46:37 AM UTC, comment #7: 

The unwind_action definitely seems like the best code construct for this particular use.  I took jwe's patch and made it slightly simpler with the use of 'auto'.  I also renamed some of the variables to attempt to make clear what is going on.  I checked things in here: http://hg.savannah.gnu.org/hgweb/octave/rev/d28016d16e9a.

I re-titled this bug report.  Using grep I made a list of the unwind_protect and unwind_protect_safe occurrences in libinterp.  There are 113 instances.  The list is attached.

I did a spot check on one instance in _eigs_.cc and the code is:


  octave::unwind_protect frame;

  frame.protect_var (call_depth);


This could be replaced with unwind_protect_var very simply.  I expect a lot of the others could also be replaced.

Besides changing any occurrences in core, we could mark the class, as we do with OCTAVE_DEPRECATED macro, so that it produces a warning during compilation.  This would be a quick way to find all instances in the code remaining after the first round of fixing on the list I generated.

(file #49874)

Rik <rik5>
Group administrator
Sun 27 Sep 2020 04:16:41 PM UTC, comment #6: 

I should also note that since you can use a lambda expression with unwind_action, it is reasonably easy to combine multiple cleanup actions in a single unwind_action object.

John W. Eaton <jwe>
Group administrator
Sun 27 Sep 2020 04:14:46 PM UTC, comment #5: 

Agree about eliminating unwind_protect first.  I've been doing that when I notice it.

I remember now that there may be a few cases where it may be a bit more difficult to eliminate.  If you are interested, search for the uses of "curr_fcn_unwind_protect_frame".  If I remember correctly, that is used to implement the "local" option for various interpreter settings (for warning state and other internal settings).

John W. Eaton <jwe>
Group administrator
Sun 27 Sep 2020 04:07:53 PM UTC, comment #4: 

The attached patch shows how you can do this job with unwind_action objects.  I fairly certain it's correct and should work, but I don't know what test to run to verify.


(file #49871)

John W. Eaton <jwe>
Group administrator
Sun 27 Sep 2020 03:54:59 PM UTC, comment #3: 

@jwe: Thanks for your comments.

I full-heartedly agree on the first point. I didn't realize (or forgot) that we have `unwind_action`. That looks like the better choice.

Wrt your second point, maybe we could check the code for places where `unwind_protect` can easily be replaced by `unwind_action`. If it turns out that we don't need `unwind_protect` anywhere, we could deprecate (or further discourage) its use.

Markus Mützel <mmuetzel>
Group administrator
Sun 27 Sep 2020 03:47:09 PM UTC, comment #2: 

Copying my comments from the discourse thread:

I recommend that you use an unwind_action object instead of unwind_protect. The unwind_action object can only perform one cleanup action, but it can already handle std::function objects and lambda expressions. Using that object is also more efficient than unwind_protect because the unwind_protect object must instantiate a std::stack object to hold the list of actions.

Separately, we might want to deprecate the unwind_protect object? It originally made sense because there was a single global stack of actions and a set of functions to operate on that stack. But then Jaroslav converted it to use the present structure in which the actions are managed on a per-scope basis using the destructor of the unwind_protect object. But we only ever need to protect a few things (at most) in a given scope, so the overhead of creating the std::stack object can be significant. In nearly all cases, it probably makes more sense to just use a set of unwind_action variables instead of a single unwind_protect object.

John W. Eaton <jwe>
Group administrator
Sun 27 Sep 2020 03:32:28 PM UTC, comment #1: 

The attached patch adds a new template to allow (non-capturing) lambda expressions.
It also changes jsonencode.cc (but I'm unsure whether it would be better to make that change in a different changeset).


(file #49869)

Markus Mützel <mmuetzel>
Group administrator
Sun 27 Sep 2020 03:23:17 PM UTC, original submission:  

In a discourse thread it was proposed to allow lambda expressions for unwind_protect objects. [1]

I'll attach a patch for that feature as soon as I get a bug number

[1]: https://octave.discourse.group/t/oddity-with-c-lambda-expressions-and-octave-code/241/3

Markus Mützel <mmuetzel>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #49896:  uprotect4.list added by rik5 (4KiB - application/octet-stream)
file #49885:  uprotect2.list added by rik5 (5KiB - application/octet-stream)
file #49878:  bug59192_graphics.cc.patch added by mmuetzel (6KiB - application/octet-stream)
file #49874:  uprotect.list added by rik5 (7KiB - application/octet-stream)
file #49871:  unwind-action-diffs.txt added by jwe (2KiB - text/plain)
file #49869:  bug59192_unwind_protect_lambda.patch added by mmuetzel (2KiB - 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 (Updated the item)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mmuetzel (Submitted the item)
  • -email is unavailable- added by mmuetzel
  •  

    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 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-09-30 rik5 Attached File- Added uprotect4.list, #49896
        StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2020-09-29 rik5 Attached File- Added uprotect2.list, #49885
    2020-09-28 mmuetzel Attached File- Added bug59192_graphics.cc.patch, #49878
    2020-09-28 rik5 Attached File- Added uprotect.list, #49874
        StatusNone In Progress
        Summaryunwind_protect objects should accept lambda expressions unwind_protect objects should be replaced with lighter weight alternatives
    2020-09-27 jwe Attached File- Added unwind-action-diffs.txt, #49871
    2020-09-27 mmuetzel Priority5 - Normal 3 - Low
    2020-09-27 mmuetzel Attached File- Added bug59192_unwind_protect_lambda.patch, #49869
    2020-09-27 mmuetzel Carbon-Copy- Added rik5

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code