Thanks for the feedback. I think the points are valid and I've been rethinking the problem.

Static storage duration and ref counting at this level makes my head hurt. I believe the extra m_count count actually is unnecessary and I made the wrong assumption about that being the problem in the first place.

The "m_rep != nil_rep ()" checks were added in the 02f0649f43d7 patch, with the motivation "Code marked as 'Bad free' by clang static analyzer." But m_count for the nil
value should never reach 0, since it starts at 1, and is increased each time in the octave_value() ctor, as I understands it.

http://hg.savannah.gnu.org/hgweb/octave/rev/02f0649f43d7

However, I think there is a theoretical underlying problem with how the octave_base_value nil is created (if I get C++ right):

octave_base_value *
octave_value::nil_rep ()
{
  static octave_base_value nr; // Initialized the first time nil_rep() is called
  return &nr;
}

If you consider the phony Octave program:

octave_value ov1 {3};  // Not calling nil_rep().
octave_value ov2 {};   // octave_base_value nr initialized with call to nil_rep().

int main (int argc, char *argv)
{
  ov1 = octave_value {}; // ov1 now has m_rep == &nr

  return 0;
  // ov2 destroyed, decreasing m_rep->m_count
  // octave_base_value nr destroyed, m_rep in ov1 becomes invalid
  // ov1 destroyed, decreasing m_rep->m_count
}

I don't think this is a practical problem, but it could be solved by something like this:

octave_base_value *
octave_value::nil_rep ()
{
  // Intentionally leaked to assure that nil's dtor is not called at program
  // termination, so that the dtor of octave_value:s don't try to change the
  // m_count of an invalid object.
  static octave_base_value *nr = new octave_base_value {};
  return nr;
}

valgrind --tool=callgrind --separate-recs=10 --dump-instr=yes --dump-after="octave::Fsingle(octave_value_list const&, int)" ./octave-cli --eval "function foo(n)
    for i = 1:n
        j = 2 + i;
    end
end
single(1);
foo(1000000)
single(1);
exit
"

Measuring with callgrind like above, before and after the patches yields 3.3% fewer instructions for the call to foo() between the call to single() and single().

It is hard to measure such small performance changes in practice, but:

function foo(n)
    for i = 1:n
        j = 2 + i;
    end
end
tic;
foo(10000000)
toc;

Measures at about a 1% reduced runtime for 4 runs each.

mean ([4.17812 4.14971 4.1426 4.16372]) / mean ([4.20858 4.20672 4.15409 4.20363])

But due to throttling issues on my laptop and arbitrary placement of things in ram I would not trust those numbers that much.


I've attached three patches. Compared to the prior patchset there is no try to inline nil_rep() and no special nil class.

33340.patch

Allocate canonical octave_base_value nil on the heap (bug #65200)

To avoid the theoretical risk of using an invalid object.

E.g. an 'octave_value ov{1234}' could be initialized before the
octave_base_value nil object, and later any '&ov = octave_value {}' would
populate it with the nil value. Then the nil value dtor would be called before
ov's dtor and ov's dtor would dereference a pointer to an invalid object in
'--m_rep->m_count == 0'.

* ov.cc: Use new operator

33341.patch

Remove redundant nil_rep() check in octave_value dtor (bug #65200)

The octave_base_value nil's reference counter should never reach 0, since its
initial ref count is 1 (and the ref count is increased by 1 in the
octave_value() ctor and decreased by one in the matching dtor).

* ov.h: Remove checks

33342.patch

Move octave_base_value() definition to header (bug #65200)

To allow for inlining the rather simple ctor and avoid function call overhead.

* ov-base.h: Define octave_base_value()
* ov-base.cc: Remove octave_base_value()

(file #55650, file #55651, file #55652)

I agree that we need better review. I also agree that - if we think the proposed change is reasonable - the first patch should go to the default branch. The second patch should go to the bytecode-interpreter branch for the time being.

Looking at the proposed changes, they look like a reasonable suggestion to me. We want only one single nilrep instance.
Maybe, we don't want to do any reference counting for that at all?

I haven't quite understood why overloading only the clone function is enough to achieve something equivalent. Or did I misunderstand the idea behind the patch?

In any case, we'll probably need to add visibility attributes where `octave_value_nilrep` is declared in ov.cc, so new types based on `octave_value` can be defined in .oct files.

Reviewed and pushed here to the bytecode branch: https://hg.savannah.gnu.org/hgweb/octave/rev/3c4c1af66055 and https://hg.savannah.gnu.org/hgweb/octave/rev/3f9ecdc396a6

'33278.patch' would be needed for merging the '33277.patch' to the bytecode interpreter branch.

33277.patch:

Special nil octave_value class (bug #65200)

octave_value_nilrep is a octave_base_value with an initial m_rep m_count
of 2, to avoid the need for nil_rep() checks in octave_value dtors.

* ov.cc: octave_nil_value
* ov.h: Remove reduntant nil checks.

33278.patch:

Special nil octave_value class (bug #65200)

* ov.cc: Define vm_dispatch_call for octave_nil_value class

(file #55600, file #55601)