bugGNU Octave - Bugs: bug #48793, no need for OCTAVE_LOCAL_BUFFER...

 
 

bug #48793: no need for OCTAVE_LOCAL_BUFFER anymore

Submitter:  CarnĂ« Draug <carandraug>
Submitted:  Mon 15 Aug 2016 08:43:10 PM UTC
   
 
Category:  Performance Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
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

Thu 20 Jul 2017 10:37:53 PM UTC, comment #11: 

Thanks for doing a code review.  The assert statement was an accidental leftover from testing.  I removed it here (http://hg.savannah.gnu.org/hgweb/octave/rev/a0b7a29338d5).

Rik <rik5>
Group administrator
Thu 20 Jul 2017 09:14:23 PM UTC, comment #10: 

Thanks for looking at this.  I suspect the difference in performance could be that getting a pointer from the chunk_buffer is a lot faster than calling new to allocate memory.  But I'm not sure it's worth the extra complexity.  It's good to see a reduction of around 300 lines without really losing anything.

Did you mean to leave the "assert (0)" in the version of the OCTAVE_LOCAL_BUFFER macro that is supposed to be used with C++14 and later?  For consistency, it should probably also not end with a semicolon.

John W. Eaton <jwe>
Group administrator
Thu 20 Jul 2017 04:18:15 PM UTC, comment #9: 

I did more benchmarking of various coding strategies including default initialization and aggregate initialization (which actually passes through to value initialization).  Over all, every strategy was within 1.5% of another.  I went with default initialization.  The resulting increase in readability of the code is huge.  See this cset (http://hg.savannah.gnu.org/hgweb/octave/rev/7b43a96c2179).

Rik <rik5>
Group administrator
Wed 19 Jul 2017 09:16:25 PM UTC, comment #8: 

Are there any alignment issues with using char and casting?

I'd be inclined to use the actual types and no casts.

John W. Eaton <jwe>
Group administrator
Wed 19 Jul 2017 08:31:06 PM UTC, comment #7: 

I'm still building for testing, but I think that using char to represent bytes is going to be the best solution.

According to this, http://en.cppreference.com/w/cpp/language/initialization, new without arguments will call default initialization.  For class objects, this would mean the default constructor.  For POD types, there is no default constructor so the value is unitialized (just what we want here).

Testing results for 'time make check'


# Original code
410.172u 28.320s 7:32.17 96.9%  0+0k 152+85440io 0pf+0w
410.452u 27.700s 7:31.92 96.9%  0+0k 32+85520io 0pf+0w

# T style
415.240u 29.312s 7:40.00 96.6%  0+0k 211312+85544io 802pf+0w
416.556u 28.180s 7:39.69 96.7%  0+0k 152+85392io 0pf+0w


So, there is a small (1.2%) but repeatable slowdown.  I'm going to try using the char technique and see if that makes a difference, but I'm getting another segfault at the moment.

Rik <rik5>
Group administrator
Wed 19 Jul 2017 07:31:43 PM UTC, comment #6: 

Thanks for fixing on stable.

As I understand it, new[] doesn't guarantee default initialization for POD types but new []() does.  You can experiment with the attached program.


(file #41249)

John W. Eaton <jwe>
Group administrator
Wed 19 Jul 2017 06:51:25 PM UTC, comment #5: 

We came across the same problem in regexp.cc.  I fixed it on the stable branch since it isn't right to have potentially segfaulting code there.  I also switched to OCTAVE_LOCAL_BUFFER_INIT so I could get rid of the for loop.  See http://hg.savannah.gnu.org/hgweb/octave/rev/fdce2b73f5ce.

For the other questions, there weren't advantages to the old system because they were never being used (#if 0 was disabling them completely at compile-time).  I was going to do performance benchmarking to see if anything has changed.  I suspect the current implementation may be a bit slower because the default constructor for object T is called N times.  The second implementation that I wrote, which uses char as the default data type to allocate bytes may be preferable.  In that case, there will still be initialization, but for a POD data type that should just be filling with 0.  There would be no calling of potentially long constructor chains.  Ideally, there is a way to just allocate storage without initialization, but that sounds like using malloc rather than new.  Maybe that is worth a question on Stack Overflow?

Rik <rik5>
Group administrator
Wed 19 Jul 2017 06:23:58 PM UTC, comment #4: 

I also came up with a similar solution for the use of unique_ptr, made it ready for C++14, and eliminated the explicit loop in the OCTAVE_LOCAL_BUFFER_INIT macro.

Does this change semantics for initialization?  Is the default constructor for objects always called now?  Will there be a significant change in performance?  I know that the chunk_buffer thing was somewhat complex, but were there advantages?

(file #41248)

John W. Eaton <jwe>
Group administrator
Wed 19 Jul 2017 06:19:38 PM UTC, comment #3: 

I just checked in a change for the regexp.cc problem:

http://hg.savannah.gnu.org/hgweb/octave/rev/99a9e19bae41

John W. Eaton <jwe>
Group administrator
Wed 19 Jul 2017 06:14:36 PM UTC, comment #2: 

I made a small mistake and wasn't pairing new[] with delete[].  The attached patch locbuf.v2.patch now compiles and the binary executes.  Running 'make check' still results in a segfault in regexp.cc, but I'm guessing that this is a real memory leak that simply hadn't been discovered before.

(file #41247)

Rik <rik5>
Group administrator
Wed 19 Jul 2017 03:23:34 PM UTC, comment #1: 

I agree that we should use standard C++ features whenever possible rather than rolling our own code.  It wasn't that hard, so I made two different implementations of OCTAVE_LOCAL_BUFFER.  One is based on bytes, and one is based on the passed in object type T.  Unfortunately, while they compile just fine, the resulting binary segfaults.  I think what this really means is that there are issues in Octave code around double frees of memory.  For reference, I'm attaching the implementation.

(file #41244)

Rik <rik5>
Group administrator
Mon 15 Aug 2016 08:43:10 PM UTC, original submission:  

Notes from an IRC discussion (I won'r have time to work on this anymore and it's probably not suitable for Octave 4.2 anyway).

The main purpose of OCTAVE_LOCAL_BUFFER is to get a pointer to a data buffer with RAII semantics.  There are comments that claim to have other optimizations.  See https://lists.gnu.org/archive/html/octave-maintainers/2008-12/msg00032.html

One of them is about allocating in the stack instead of the heap for small chunks but that is actually disabled http://hg.savannah.gnu.org/hgweb/octave/file/8192c26fcda4/liboctave/util/oct-locbuf.h#l174


#if 0 // defined (HAVE_DYNAMIC_AUTO_ARRAYS)


We can't use std::vector because its specialization for bool doesn't allow to get "bool *".

std::unique_ptr is a better choice. On mtmx words:

> primary purpose of OCTAVE_LOCAL_BUFFER was to have very lightweight memory buffer that just has a desctructor, so maybe the best alternative would be std::unique_ptr<T> instead of std::vector<T>? or std::array<T> where the size is completely fixed/known at compile time


I tried to do it with:


#define OCTAVE_LOCAL_BUFFER(T, buf, size)            \
  std::unique_ptr<T> _buffer_ ## buf (new T(size));  \
  T* buf = _buffer_ ## buf.get ();

 #define OCTAVE_LOCAL_BUFFER_INIT(T, buf, size, value) \
  OCTAVE_LOCAL_BUFFER(T, buf, size)                   \
  for (size_t __i__ = 0; __i__ < size; __i__++)       \
    buf[__i__] = value;


But got some with const char*

This will still need to work because of other packages, including with bool (used in the tisean and control package). But we should deprecate and replace with standard C++ features that do it for us.

Carnë Draug <carandraug>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #41249:  opnew.cc added by jwe (567B - text/x-c++src)
file #41248:  revised-locbuf-patch.txt added by jwe (8KiB - text/plain)
file #41247:  locbuf.v2.patch added by rik5 (13KiB - text/x-patch)
file #41244:  local_buffer.patch added by rik5 (13KiB - 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 jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by siko1056 (Updated the item)
  • -email is unavailable- added by siko1056 (interested)
  • -email is unavailable- added by carandraug (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 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-07-20 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2017-07-19 jwe Attached File- Added opnew.cc, #41249
    2017-07-19 jwe Attached File- Added revised-locbuf-patch.txt, #41248
    2017-07-19 rik5 Attached File- Added locbuf.v2.patch, #41247
    2017-07-19 rik5 Attached File- Added local_buffer.patch, #41244
        StatusNeed Info Confirmed
        Carbon-Copy- Added jwe
    2016-08-23 siko1056 CategoryNone Performance
        Item GroupNone Feature Request
        StatusNone Need Info
    2016-08-16 siko1056 Carbon-Copy- Added siko1056

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code