Re: [Development] Question about the QDataBuffer from src/gui/painting
On Monday 26 October 2015 09:41:00 Gunnar Sletta wrote: > Also, given the purpose of this class, then using QDataBuffer as an > on-the-stack member in a function (like in mergePoints()) will pretty much > always be wrong. There is no point in using a pool when it is discarded > after a single use, after all. Yes, QVarLengthArray would be preferable in many cases. -- Marc Mutz | Senior Software Engineer KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company Tel: +49-30-521325470 KDAB - The Qt Experts ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Question about the QDataBuffer from src/gui/painting
> On 26 Oct 2015, at 10:12, Marc Mutz wrote: > > On Monday 26 October 2015 08:20:38 Gunnar Sletta wrote: >> The purpose of QDataBuffer is to provide a managed pool of memory that can >> grow, but does not shrink unless shrunk explicitly. QVector is unusable >> for this purpose as a resize/clear/append would cause a lot of >> reallocations for the places where QDataBuffer is used. > > QDataBuffer uses realloc(), which of course std::vector cannot. With a > geometric growth strategy, this doesn't matter much, and most types held in > QDataBuffer are PODs (maybe not in C++98, but in C++11), but if it turns out > that it does matter, then fixing QVector to not shed excess capacity at the > first opportunity would be the right thing to do. I can't remember why we used realloc. This class was written as part of the paint system in 4.0, so it is a long time ago. I agree that it doesn't look like it is needed. > That said, neither QVector nor std::vector will match the performance of > QDataBuffer, which declines to care about such basic things as copying and > safe > appends from an already-existing element or Ts which aren't Q_PRIMITIVE_TYPE > (it should assert that, btw). True, it was never intended to be generic copyable container, just a managed memory pool to help with reallocations. > But either (QVector w/fixed capacity management or std::vector) should be > fast > enough. Surely, the ported code will be faster, because stuff like calling > points() over and over in QPathSegments::mergePoints() will be found and > fixed. I don't think changing QVector is the right thing. That is bound to have side effects elsewhere, so I don't like that. std::vector wasn't an option back when the class was written, but it that was a long time ago. Looks more sensible now. Also, given the purpose of this class, then using QDataBuffer as an on-the-stack member in a function (like in mergePoints()) will pretty much always be wrong. There is no point in using a pool when it is discarded after a single use, after all. > > Thanks, > Marc > > -- > Marc Mutz | Senior Software Engineer > KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company > Tel: +49-30-521325470 > KDAB - The Qt Experts > ___ > Development mailing list > Development@qt-project.org > http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Question about the QDataBuffer from src/gui/painting
> On 26 Oct 2015, at 09:56, Marc Mutz wrote: > > On Monday 26 October 2015 08:20:38 Gunnar Sletta wrote: >> I don't know if std::vector::erase() and std::vector::clear() can guarantee >> that the data is not reallocated, even if reserve() has been specified. If >> that can be guaranteed, removing the class would be fine by me. > > Since std::vector::erase(f,l) is guaranteed to preserve iterators and > references to elements before f, no reallocation can occur (unless f == > begin()). Yeah, but that is kinda the point. I wanted a pool that I could reset and start over again and again and it would only reallocate when a previous pool size was exceeded. Using std::vector::erase() would mean a realloc could happen when the thing is cleared. > > For clear(), that's not guaranteed, but is a common optimisation. So much so > that Scott Meyers has an item about how to shed excess capacity in Effective > STL. > > We could add a testcase: > > std::vector v; > v.resize(1024); > v.clear(); > QvERIFY(v.capacity() >= 1024); If that passes on all our compilers, then I'm all for switching. > > Thanks, > Marc > > -- > Marc Mutz | Senior Software Engineer > KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company > Tel: +49-30-521325470 > KDAB - The Qt Experts > ___ > Development mailing list > Development@qt-project.org > http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Question about the QDataBuffer from src/gui/painting
On Monday 26 October 2015 08:20:38 Gunnar Sletta wrote: > The purpose of QDataBuffer is to provide a managed pool of memory that can > grow, but does not shrink unless shrunk explicitly. QVector is unusable > for this purpose as a resize/clear/append would cause a lot of > reallocations for the places where QDataBuffer is used. QDataBuffer uses realloc(), which of course std::vector cannot. With a geometric growth strategy, this doesn't matter much, and most types held in QDataBuffer are PODs (maybe not in C++98, but in C++11), but if it turns out that it does matter, then fixing QVector to not shed excess capacity at the first opportunity would be the right thing to do. That said, neither QVector nor std::vector will match the performance of QDataBuffer, which declines to care about such basic things as copying and safe appends from an already-existing element or Ts which aren't Q_PRIMITIVE_TYPE (it should assert that, btw). But either (QVector w/fixed capacity management or std::vector) should be fast enough. Surely, the ported code will be faster, because stuff like calling points() over and over in QPathSegments::mergePoints() will be found and fixed. Thanks, Marc -- Marc Mutz | Senior Software Engineer KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company Tel: +49-30-521325470 KDAB - The Qt Experts ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Question about the QDataBuffer from src/gui/painting
On Monday 26 October 2015 08:20:38 Gunnar Sletta wrote: > I don't know if std::vector::erase() and std::vector::clear() can guarantee > that the data is not reallocated, even if reserve() has been specified. If > that can be guaranteed, removing the class would be fine by me. Since std::vector::erase(f,l) is guaranteed to preserve iterators and references to elements before f, no reallocation can occur (unless f == begin()). For clear(), that's not guaranteed, but is a common optimisation. So much so that Scott Meyers has an item about how to shed excess capacity in Effective STL. We could add a testcase: std::vector v; v.resize(1024); v.clear(); QvERIFY(v.capacity() >= 1024); Thanks, Marc -- Marc Mutz | Senior Software Engineer KDAB (Deutschland) GmbH & Co.KG, a KDAB Group Company Tel: +49-30-521325470 KDAB - The Qt Experts ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] Question about the QDataBuffer from src/gui/painting
The purpose of QDataBuffer is to provide a managed pool of memory that can grow, but does not shrink unless shrunk explicitly. QVector is unusable for this purpose as a resize/clear/append would cause a lot of reallocations for the places where QDataBuffer is used. I don't know if std::vector::erase() and std::vector::clear() can guarantee that the data is not reallocated, even if reserve() has been specified. If that can be guaranteed, removing the class would be fine by me. cheers, Gunnar > On 25 Oct 2015, at 20:28, Maks Naumov wrote: > > In my small change https://codereview.qt-project.org/#/c/138795/ > Marc Mutz wrote: "This class deserves to die. If you do not believe, copy an > instance and you'll see." > > I agree with him, but it will be quite a big change because it is used in > many places. > https://github.com/qtproject/qtbase/search?utf8=%E2%9C%93&q=QDataBuffer > > QDataBuffer is a small version of std::vector with disabilities and the > constructor > with one parameter(array size). It is intended only for the primitive data > types. > https://github.com/qtproject/qtbase/blob/5.6/src/gui/painting/qdatabuffer_p.h > > What do you think is it worth to leave QDataBuffer and update it a little? > For example to add Q_DECL_NOEXCEPT, Q_DECL_CONSTEXPR ..., > Type * buffer replace with QScopedPointer with QScopedPointerPodDeleter? > Or replace QDataBeffer where it is used by the std::vector, of course, > if it doesn't hurt performance? > ___ > Development mailing list > Development@qt-project.org > http://lists.qt-project.org/mailman/listinfo/development ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
[Development] Question about the QDataBuffer from src/gui/painting
In my small change https://codereview.qt-project.org/#/c/138795/ Marc Mutz wrote: "This class deserves to die. If you do not believe, copy an instance and you'll see." I agree with him, but it will be quite a big change because it is used in many places. https://github.com/qtproject/qtbase/search?utf8=%E2%9C%93&q=QDataBuffer QDataBuffer is a small version of std::vector with disabilities and the constructor with one parameter(array size). It is intended only for the primitive data types. https://github.com/qtproject/qtbase/blob/5.6/src/gui/painting/qdatabuffer_p.h What do you think is it worth to leave QDataBuffer and update it a little? For example to add Q_DECL_NOEXCEPT, Q_DECL_CONSTEXPR ..., Type * buffer replace with QScopedPointer with QScopedPointerPodDeleter? Or replace QDataBeffer where it is used by the std::vector, of course, if it doesn't hurt performance? ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
