bugGNU Octave - Bugs: bug #65200, Special nil octave_base_value...

 
 

bug #65200: Special nil octave_base_value class to speed up octave_value dtors

Submitter:  Petter <petter>
Submitted:  Tue 23 Jan 2024 10:40:19 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Performance
Status:  In Progress Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 01 Feb 2024 12:41:47 AM UTC, comment #8: 


> 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?


The reasoning was that the special nil class should return it self on a clone(), not return a octave_base_value as it would if the clone() function was not overloaded, to save a construction of an object.

But I've backtracked on increasing m_count with 1 solving any problem, so it shouldn't matter since the class is unnecessary.

> In the nil_rep changeset, what is meant by the comment "// Also increased in octave_base_value()"?  I don't see any change to the octave_base_value default constructor.


I should have written "Is set to 1 in octave_base_value". I tried to convey it would end up at 2. But I realized I was wrong about that being a problem in the first place.

Petter <petter>
Thu 01 Feb 2024 12:17:26 AM UTC, comment #7: 

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)

Petter <petter>
Thu 25 Jan 2024 04:42:34 PM UTC, comment #6: 

In the nil_rep changeset, what is meant by the comment "// Also increased in octave_base_value()"?  I don't see any change to the octave_base_value default constructor.

How much does this really save?  Could we achieve the same thing (avoiding the "m_rep != nil_rep ()" test in by modifying the nil_rep function to be


octave_base_value * octave_value::nil_rep ()
{
  static bool initialized = false;
  static octave_base_value nr;

  if (! initialized)
    {
      // Artificially increment count for NR so that it should never
      // reach zero and NR will never be a candidate for deletion.

      nr.m_count++;
      initialized = true;
    }

  return &nr;
}


If using a separate class is better, then why is it better?  If the reason is performance, is it actually a measurable difference?

If we do decide use the class, I don't think the reinterpret_cast is needed, or it wouldn't be if the full declaration of the octave_nil_value class were available in ov.h and if it used public inheritance.  Is there some reason the full declaration can't be available there and a reason to use private inheritance?  Also, make the octave_value_nilrep object a static member of the octave_nil_value class instead of a global variable.

We should also define or explicitly default or delete the common constructors, operators, and destructor.  Finally, the grammar of the comment for the octave_nil_value class should be fixed and it would be good if the commit message also follows the project style guidelines.

John W. Eaton <jwe>
Group administrator
Thu 25 Jan 2024 10:18:07 AM UTC, comment #5: 

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.


Markus M├╝tzel <mmuetzel>
Group administrator
Thu 25 Jan 2024 03:28:12 AM UTC, comment #4: 

What review?

Before pushing changes that will eventually affect more than just the bytecode interpreter, we need to have some actual review of the code before we make a decision about whether it to accept it.  And, if accepted, the changes should probably go on default.  I'd rather make that decision now instead of waiting until we someday merge the bytecode interpreter branch back to default, at which point we will probably have forgotten about these changes and won't remember to check whether they are really appropriate for all of Octave or just for the bytecode interpreter.

Even for code that is just in the bytecode interpreter files, we probably need more review than we have been doing.

Maybe we do want this change to the way we handle octave_value::nil_rep, but until we have some discussion about it, I've backed out the changes.

John W. Eaton <jwe>
Group administrator
Thu 25 Jan 2024 01:32:56 AM UTC, comment #3: 
Arun Giridhar <arungiridhar>
Group Member
Tue 23 Jan 2024 11:06:24 PM UTC, comment #2: 

I spotted and removed some trailing spaces. The same patches again.

(file #55602, file #55603)

Petter <petter>
Tue 23 Jan 2024 10:44:30 PM UTC, comment #1: 

'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)

Petter <petter>
Tue 23 Jan 2024 10:40:19 PM UTC, original submission:  

This patch adds a special nil octave_base_value class with the sole purpose of having an initial m_count of 2, to avoid needing to check for the nil_rep() in octave_value dtors.

(The nil value is in file scope and can't be deleted.)

A loop like:

  for i = 1:n
    2 + i;
  end

executes in 10% fewer instructions according to callgrind.

I'll post the patches in a follow up post to get the bug nr.

Petter <petter>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #55650:  octave_33440.patch added by petter (1KiB - text/x-patch)
file #55651:  octave_33441.patch added by petter (2KiB - text/x-patch)
file #55652:  octave_33442.patch added by petter (2KiB - text/x-patch)
file #55602:  octave_33277.patch added by petter (3KiB - text/x-patch)
file #55603:  octave_33278.patch added by petter (831B - text/x-patch)
file #55600:  octave_33277.patch added by petter (3KiB - text/x-patch)
file #55601:  octave_33278.patch added by petter (831B - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by arungiridhar (Posted a comment)
  • -email is unavailable- added by petter (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 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-02-01 petter Attached File- Added octave_33440.patch, #55650
        Attached File- Added octave_33441.patch, #55651
        Attached File- Added octave_33442.patch, #55652
    2024-01-25 jwe StatusReady For Test In Progress
    2024-01-25 arungiridhar StatusNone Ready For Test
    2024-01-23 petter Attached File- Added octave_33277.patch, #55602
        Attached File- Added octave_33278.patch, #55603
    2024-01-23 petter Attached File- Added octave_33277.patch, #55600
        Attached File- Added octave_33278.patch, #55601

    Back to the top

    Powered by Savane 3.12.
    Corresponding source code