bugGNU Octave - Bugs: bug #59500, sub-classes of octave_base_value...

 
 

bug #59500: sub-classes of octave_base_value aren't assignable

Submitter:  None
Submitted:  Sun 22 Nov 2020 03:24:21 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Confirmed Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  * Open Release:  * dev
Operating System:  * Any Fixed Release:  None
Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 29 Nov 2020 11:58:10 PM UTC, comment #9: 

There are a lot of subtle issues here.  I can see, and indeed verified, that adding the CAO function that you posited allows the code example from this report to compile.  However, I don't feel that I have enough knowledge of Octave's internals to say whether this is a good/bad/neutral addition to Octave.  Probably only jwe can make that decision.

Rik <rik5>
Group administrator
Fri 27 Nov 2020 09:05:05 PM UTC, comment #8: 


struct refcount
{
  refcount (int v) : i(v) {}
  refcount (const refcount&) = delete;
  refcount& operator = (const refcount&) = delete;
  int i;
};

struct octave_base_value
{
  octave_base_value () : count (1) {}
  octave_base_value (const octave_base_value&) : count (1) {}
  refcount count;
};

struct octave_scalar : octave_base_value
{
  double scalar;
};
struct octave_diag : octave_base_value
{

};


I can explain more. In the above example I provided a minimal example that resembles the current (Octave 6.1) implementation of the classes related to this thread.
Please see the struct 'refcount'. Its copy constructor (CC) and copy assignment operator (CAO) are explicitly deleted.
Now see the struct 'octave_base_value'. It has a member named 'count' that is of class 'refcount'. Because the refcount class has deleted CC and CAO the default CC and CAO of octave_base_value also are automatically deleted. But we can define custom CC and CAO for octave_base_value to override the default behavior and enabling copying and assignment.
As we can see the current implementation of octave_base_value has a custom copy constructor. Without the custom copy constructor octave_base_value and all its subclasses would not be copyable.
we also can see that in the copy constructor the count member isn't copied instead it gets the value 1. It allows to copy the data contents of octave objects (of subclasses of octave_base_value) without copying the reference count.
In other words one can use such expression:


octave_scalar a;
octave_scalar b(a);


But octave_base_value doesn't have a custom copy assignment operator. Therefore it is currently impossible to write such expressions:


octave_base_value a;
octave_base_value b;
b = a;

octave_scalar c;
octave_scalar d;
d = c;


What I suggested is a minimal change that is required to enable copy assignment of octave_base_value and its subclasses.


octave_base_value& operator = (const octave_base_value&) { return *this; }


While it seems that it does nothing but it actually does something. It, like the copy constructor, doesn't copy the refcount. It overrides the default behavior and enables assigning to octave_base_value and it subclasses.
The following example is invalid and won't compile:
 

octave_scalar a;
octave_diag b;

a = b;


Because octave_scalar and octave_diag are different types and octave_scalar doesn't overload an assignment operator with an argument of type octave_diag.
However every class can define its own assignment operator. But I think it is unneeded and the definition for octave_base_value is sufficient.

Some questions and answers:

Q: Is the decision for deleting CC and CAO of refcount in Octave 6.1 is a correct decision?
A: Yes, I agree that the refcount shouldn't be copyable ans assignable.

Q: What are the outcomes of the new implementation of refcout?
A: Since the refcount is used in some classes other than octave_base_value they now aren't copyable nor assignable. In a quick review I noticed that much of them are private classes and nothing is required to be done. But I think 'base_property' in libinterp/corefcn/graphic.in.h needs a custom assignment operator.

Q: Is defining the custom assignment operator of octave_base_value needed for the interpreter to work correctly?
A: No, the interpreter works perfectly without the custom assignment operator.

Q: What percentage of the users/developers may need/use/think about such a feature?
A: I cannot estimate but I know at least one person! They can life without that feature but with a little overhead!

Anonymous
Fri 27 Nov 2020 05:13:04 PM UTC, comment #7: 

Can copy assignment operator declared at the octave_base_value level actually work?  Multiple classes inherit from octave_base_value and the class itself is mostly virtual methods meant to be re-defined.  As an example, octave_base_scalar and octave_base_diag both inherit from octave_base_value().  Assuming copy assignment at octave_base_value is what you defined


octave_base_value& operator = (const octave_base_value&) { return *this; }


then this should theoretically compile


octave_base_scalar a;
octave_base_diag b;

a = b;


But, I don't see how it would work because octave_base_scalar only has room for one value and b requires a matrix worth of storage.

Would it make sense to define copy assignment at the leaf classes instead?  That way an octave_scalar_value<double> could be assigned to an octave_scalar_value<double> but if the assignment was across classes, such as octave_scalar_value<int>, a new value would have to be created.

Rik <rik5>
Group administrator
Thu 26 Nov 2020 05:27:46 AM UTC, comment #6: 

An assignment operator is needed to allow copying the data contents of an object and disallow copying the reference count. The proposed assignment operator for octave_base_value in comment #3 does that. Setting the assignment operator of octave_base_value to default won't compile because it tries to assign to the 'refcount' that isn't by definition assignable.
 
 One may argue that if someone defines 'myfunction' as:
 

 octave_value_list
myfunction (const octave_value_list& lst, int n = 0)
{
  static octave_value val = args(0);
  return ovl ();
}


ans uses  `*static_cast<octave_scalar*> (lst(0).internal_rep ()) = double(i);` it leads to an unwanted result and in the next iterations the static variable is changed. One can prevent such behavior by defining a second octave_value:
 

octave_value_list lst = ovl (octave_value (0.0));

octave_value tmp(0.0);

for (int i = 0; i < 1000000; i++)
  {
    *static_cast<octave_scalar*> (tmp.internal_rep ()) = double(i);

    lst(0) = tmp;

    myfunction (lst);
  }


Another solution is checking the reference count:


octave_value_list lst = ovl (octave_value (0.0));

for (int i = 0; i < 1000000; i++)
  {
    if (lst(0).get_count () > 1)
      lst(0) = double(i);
    else
      *static_cast<octave_scalar*> (lst(0).internal_rep ()) = double(i);

    myfunction (lst);
  }


However if the decision is keeping the current behavior I suggest to make octave_value behave faster for lightweight objects (possibly embedding small objects directly in octave_value). If it is OK I may upload a patch that implements such structure. This strategy may be extended to small arrays and lists.

Anonymous
Tue 24 Nov 2020 03:54:26 PM UTC, comment #5: 

If any change is made, I think we want to enforce the copy-on-write semantics of octave_value and octave_value_list objects.

John W. Eaton <jwe>
Group administrator
Tue 24 Nov 2020 01:32:46 AM UTC, comment #4: 

I agree that having a copy assignment operator for the octave_base_value class should resolve the issue.  I don't know yet whether explicit declaration is the best strategy or whether it should be set to the keyword "default" so that a default is generated.

On the first point, I believe 'x' is an object within the data container 'lst'.  Changing the value of 'x' will therefore be reflected in the container passed to myfcn.  But, this is subject to test after the compilation error has been resolved.


Rik <rik5>
Group administrator
Mon 23 Nov 2020 06:26:34 PM UTC, comment #3: 

Thanks for reply!
The point is that the purpose is passing the iteration variable `i` via `lst` to the function. Assigning to the `x` in the loop doesn't solve the problem because `x` isn't a member of `lst`.

I think defining a copy assignment operator for libinterp/octave-value/octave_base_value may solve the problem:


octave_base_value& operator = (const octave_base_value&) { return *this; }


Anonymous
Mon 23 Nov 2020 04:43:38 PM UTC, comment #2: 

I think this is an unintended side effect of internal code re-organization.  I can't see any reason why it should not be possible to declare two octave_scalar variables and then assign one to the other.  It does seem to me like that requires fixing.

However, I don't think it's a good idea to rely on the form of Octave internals for higher performance, as the internals may change at any time.  I would advise coding to the API as that can be relied on between versions (5.2 to 6.1 for example).  When a change is going to be made to the API, we announce it and preserve functionality for two versions of Octave so that there is sufficient time to change code before it becomes broken code.

In the example you posted, isn't it enough to declare an octave_scalar ahead of the for loop and then re-use it rather than creating a temporary every time?


octave_scalar x (0.0);
octave_value_list lst = ovl (x);

for (int i = 0; i < 1000000; i++)
  {
    x = double(i);
    myfunction (lst);
  }


Rik <rik5>
Group administrator
Sun 22 Nov 2020 03:27:36 PM UTC, comment #1: 

I attached the loop example bug_59500.cc.

(file #50313)

Anonymous
Sun 22 Nov 2020 03:24:21 PM UTC, original submission:  

I recently noticed that all sub-classes of `octave_base_value` aren't copy-assignable. For example:


octave_scalar a(1.0);
octave_scalar b(2.0);
b = a;


The example works on Octave 5.2.0 but in 6.0.92 doesn't work.
The compiler says that the `count` member of `octave_base_value` is of class `octave::refcount<octave_idx_type>` and the copy assignment operator of `octave::refcount<octave_idx_type>` is explicitly deleted. Because of that the copy assignment operator of `octave_base_value` is also implicitly deleted and because `octave_scalar` is a sub-class of `octave_base_value` and it doesn't defined its own copy assignment operator its default assignment operator is also deleted.

Consider a typical signature of builtin functions:


octave_value_list
myfunction (const octave_value_list& lst, int n = 0)
{}


Now I want to use this function in a loop. One way is:


octave_value_list lst = ovl (octave_value (0.0));

for (int i = 0; i < 1000000; i++)
  {
    lst(0) = double(i);
    myfunction (lst);
  }


The statement `lst(0) = double(i);` calls the constructor of `octave_value::octave_value(double d)` and it internally calls `new  octave_scalar(d)`. So creating a scalar value needs at least a memory allocation.
A second way to doing the task is:


octave_value_list lst = ovl (octave_value (0.0));
for (int i = 0; i < 1000000; i++)
  {
    *static_cast<octave_scalar*> (lst(0).internal_rep ()) = double(i);

    myfunction (lst);
  }


I prefer and use the second method because it doesn't need extra 1000000 memory allocations.
Unfortunately the second method doesn't work in new alpha 6.0.92 version.

Many Thanks!


Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #50313:  bug_59500.cc added by None (517B - 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 jwe (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by rik5 (Posted a comment)
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2020-11-29 rik5 Release6.0.92 dev
    2020-11-24 rik5 Carbon-Copy- Added jwe
    2020-11-23 rik5 StatusNone Confirmed
    2020-11-22 None Attached File- Added bug_59500.cc, #50313

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code