Re: [Development] QVector reserve counterproductive?
On 2018-03-04 13:20, Christian Ehrlicher wrote: Am 04.03.2018 um 10:03 schrieb Christian Ehrlicher: Am 03.03.2018 um 23:22 schrieb Martins, Sérgio: On 2018-03-03 20:38, Christian Ehrlicher wrote: But it looks like reserve() allocates *exactly* the amount of elements given. Actually that qpainterpath code is off-by-one, it should be: d_func()->elements.reserve(d_func()->elements.size() + polygon.size() - 1); which also fixes the performance problem (by luck, in that specific benchmark). Please test. You're correct - this fixes the problem. Looks like the growth strategy is only applied when the current capacity == current size and not when the current capacity is slightly higher... good to know (and should maybe be documented?) The off-by-one idea was wrong. moveTo() will add another point to elements - therefore reserve(d_func()->elements.size() + polygon.size()) is correct. Maybe the best idea here would be to simply remove the reserve() calls and let QVector do what it is made for... :) If you're sure you're not pessimizing any other case, then removing reserve() is fine. But maybe there's a third option: vec.reserve(qMax(needed, what_the_capacity_would_have_been_without_reserve)) this way it's good for short containers, as growth is bootstrapped and also good for big containers as reserving becomes a no-op, as there's already capacity. Regards, -- Sérgio Martins | sergio.mart...@kdab.com | Senior Software Engineer Klarälvdalens Datakonsult AB, a KDAB Group company Tel: Sweden (HQ) +46-563-540090, USA +1-866-777-KDAB(5322) KDAB - The Qt, C++ and OpenGL Experts ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] QVector reserve counterproductive?
Am 04.03.2018 um 10:03 schrieb Christian Ehrlicher: Am 03.03.2018 um 23:22 schrieb Martins, Sérgio: On 2018-03-03 20:38, Christian Ehrlicher wrote: But it looks like reserve() allocates *exactly* the amount of elements given. Actually that qpainterpath code is off-by-one, it should be: d_func()->elements.reserve(d_func()->elements.size() + polygon.size() - 1); which also fixes the performance problem (by luck, in that specific benchmark). Please test. You're correct - this fixes the problem. Looks like the growth strategy is only applied when the current capacity == current size and not when the current capacity is slightly higher... good to know (and should maybe be documented?) The off-by-one idea was wrong. moveTo() will add another point to elements - therefore reserve(d_func()->elements.size() + polygon.size()) is correct. Maybe the best idea here would be to simply remove the reserve() calls and let QVector do what it is made for... :) Christian ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] QVector reserve counterproductive?
Am 03.03.2018 um 23:22 schrieb Martins, Sérgio: On 2018-03-03 20:38, Christian Ehrlicher wrote: But it looks like reserve() allocates *exactly* the amount of elements given. Actually that qpainterpath code is off-by-one, it should be: d_func()->elements.reserve(d_func()->elements.size() + polygon.size() - 1); which also fixes the performance problem (by luck, in that specific benchmark). Please test. You're correct - this fixes the problem. Looks like the growth strategy is only applied when the current capacity == current size and not when the current capacity is slightly higher... good to know (and should maybe be documented?) Will create a patch for qpainterpath soon. Thx, Christian ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
Re: [Development] QVector reserve counterproductive?
On 2018-03-03 20:38, Christian Ehrlicher wrote: Hi, while digging through the bugreports I found https://bugreports.qt.io/browse/QTBUG-66677 which seems to show a downside of QVector::reserve(). Yes, reserve() shouldn't be called repeatedly inside loops. Instead use it only once, outside your outermost loop. QPainterPath::addPolygon() is calling reserve() to make sure to have enough space for the new positions. This is exactly what I would have done and what e.g. CLazy suggests. Not really, see "Example where reserve shouldn't be used" in https://github.com/KDE/clazy/blob/master/src/checks/level2/README-reserve-candidates.md and also the pitfalls section where it talkes about using the growth strategy instead But it looks like reserve() allocates *exactly* the amount of elements given. Actually that qpainterpath code is off-by-one, it should be: d_func()->elements.reserve(d_func()->elements.size() + polygon.size() - 1); which also fixes the performance problem (by luck, in that specific benchmark). Please test. Ideas welcome on how we can catch more of these cases... Regards, -- Sérgio Martins | sergio.mart...@kdab.com | Senior Software Engineer Klarälvdalens Datakonsult AB, a KDAB Group company Tel: Sweden (HQ) +46-563-540090, USA +1-866-777-KDAB(5322) KDAB - The Qt, C++ and OpenGL Experts ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
[Development] QVector reserve counterproductive?
Hi, while digging through the bugreports I found https://bugreports.qt.io/browse/QTBUG-66677 which seems to show a downside of QVector::reserve(). QPainterPath::addPolygon() is calling reserve() to make sure to have enough space for the new positions. This is exactly what I would have done and what e.g. CLazy suggests. But it looks like reserve() allocates *exactly* the amount of elements given. Therefore calling addPolygon() a second time will do a new reallocation and so on. The documentation for reserve() only states that memory for 'at least size elements' is allocated so my naive assumption would be that calling reserve a second time with a slightly bigger value than before does nothing. But this doesn't seem to be the case here. For the bug: removing the reserve() inside addPolygon() fixes the speed issues mentioned in the bug report. Don't know if the bug report is a valid use case though. Thx, Christian ___ Development mailing list Development@qt-project.org http://lists.qt-project.org/mailman/listinfo/development
