[Octave-bug-tracker] [bug #65200] Special nil octave_base_value class to speed up octave_value dtors

[Petter]

Follow-up Comment #7, bug#65200 (group octave):

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)

    _______________________________________________________

Additional Item Attachment:

File name: octave_33440.patch             Size:1 KB
    <https://file.savannah.gnu.org/file/octave_33440.patch?file_id=55650>

File name: octave_33441.patch             Size:1 KB
    <https://file.savannah.gnu.org/file/octave_33441.patch?file_id=55651>

File name: octave_33442.patch             Size:2 KB
    <https://file.savannah.gnu.org/file/octave_33442.patch?file_id=55652>


    AGPL NOTICE

These attachments are served by Savane. You can download the corresponding
source code of Savane at
https://git.savannah.nongnu.org/cgit/administration/savane.git/snapshot/savane-d60fd21b9fdc4361416fbef3ef8da7a22572ea99.tar.gz


    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?65200>

_______________________________________________
Message sent via Savannah
https://savannah.gnu.org/