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).
|
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.
|
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).
|
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.
|
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'
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.
|
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)
|
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?
|
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)
|
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
|
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)
|
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)
|
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
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:
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.
|