From d2e0532b1259432f1598b939c904cb9e0b1768f7 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Sat, 12 Sep 2026 18:00:01 +1200 Subject: [PATCH] Cache the drawing of element definitions that have no uuid ElementPictureFactory caches the QPicture it builds for an element definition, keyed by that definition's uuid. Definitions saved before uuids were written do not have one, and every one of them presented the same null uuid. getPictures() spotted that and took an uncached path, so the drawing was rebuilt from the XML for every instance the project placed. Counted on the shipped examples: examples/m_000.qet 831 builds for 97 definitions examples/affuteuse_250h.qet 256 builds for 106 definitions examples/industrial.qet 65 builds, 553 cache hits (has uuids) 13 of the 23 example projects carry definitions without a uuid, so this is not a rare shape. Derive a key from the location when the definition has no uuid of its own. ElementsLocation::toString() qualifies an embedded path with the id of the project owning it, and QETApp hands out project ids from an ever-increasing counter and never reuses them, so the derived key cannot collide with an element of another project. Measured with callgrind, which counts instructions and so does not depend on what else the machine is doing, opening examples/affuteuse_250h.qet: 4,801,381,735 -> 4,285,411,908 instructions (-10.7 %) ElementPictureFactory::build 995 M -> 478 M ElementPictureFactory::getPictures 1289 M -> 774 M The halving of build() matches the counters independently: 106 definitions against 256 instances is 41 %, and the cost falls to 48 %. This also retires a latent aliasing bug rather than a measured one: build() inserted into m_primitives_H under the same null uuid for every definition lacking one, and getPrimitives() read back through that shared key. Its only caller is the image export dialog, which the command line does not reach, so no wrong output could be demonstrated here -- but the entries could only ever have belonged to whichever element was built last. --info stays byte identical on all 23 example projects, and the SVG export of affuteuse_250h.qet -- a project whose definitions all lack uuids -- is byte identical too. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01L6MRq2Ach1ogvnGcbuqNLr --- sources/factory/elementpicturefactory.cpp | 47 ++++++++++++++++------- sources/factory/elementpicturefactory.h | 1 + 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/sources/factory/elementpicturefactory.cpp b/sources/factory/elementpicturefactory.cpp index cf0d89bf6..b8de3cf38 100644 --- a/sources/factory/elementpicturefactory.cpp +++ b/sources/factory/elementpicturefactory.cpp @@ -36,6 +36,33 @@ ElementPictureFactory* ElementPictureFactory::m_factory = nullptr; +/** + @brief ElementPictureFactory::cacheKey + @param location + @return the key under which the drawing of the element at location is + cached. + + An element definition normally carries its own uuid, and that is used + directly. Definitions saved before uuids were written do not have one -- + a good share of the shipped example projects are still in that state -- + and they all presented the same null uuid as a key. The drawing of such + an element was therefore either rebuilt for every instance placed, or + stored under a key it shared with every other element lacking a uuid. + Derive a stable key from the location for those instead: + ElementsLocation::toString() qualifies an embedded path with the id of + the project that owns it, and project ids come from an ever-increasing + counter and are never reused, so the derived key cannot collide with an + element of another project. +*/ +QUuid ElementPictureFactory::cacheKey(const ElementsLocation &location) +{ + const QUuid uuid = location.uuid(); + if (!uuid.isNull()) { + return uuid; + } + return QUuid::createUuidV5(QUuid(), location.toString().toUtf8()); +} + /** @brief ElementPictureFactory::getPictures Set the picture of the element at location. @@ -50,12 +77,7 @@ void ElementPictureFactory::getPictures(const ElementsLocation &location, QPictu return; } - QUuid uuid = location.uuid(); - if(Q_UNLIKELY(uuid.isNull())) - { - build(location, &picture, &low_picture); - return; - } + const QUuid uuid = cacheKey(location); if(m_pictures_H.contains(uuid)) { @@ -80,7 +102,7 @@ void ElementPictureFactory::getPictures(const ElementsLocation &location, QPictu */ QPixmap ElementPictureFactory::pixmap(const ElementsLocation &location) { - QUuid uuid = location.uuid(); + const QUuid uuid = cacheKey(location); if (m_pixmap_H.contains(uuid)) { return m_pixmap_H.value(uuid); @@ -114,9 +136,7 @@ QPixmap ElementPictureFactory::pixmap(const ElementsLocation &location) painter.translate(hsx, hsy); painter.drawPicture(0, 0, m_pictures_H.value(uuid)); - if (!uuid.isNull()) { - m_pixmap_H.insert(uuid, pix); - } + m_pixmap_H.insert(uuid, pix); return pix; } @@ -132,10 +152,11 @@ QPixmap ElementPictureFactory::pixmap(const ElementsLocation &location) ElementPictureFactory::primitives ElementPictureFactory::getPrimitives( const ElementsLocation &location) { - if(!m_primitives_H.contains(location.uuid())) + const QUuid uuid = cacheKey(location); + if(!m_primitives_H.contains(uuid)) build(location); - return m_primitives_H.value(location.uuid()); + return m_primitives_H.value(uuid); } ElementPictureFactory::~ElementPictureFactory() @@ -270,7 +291,7 @@ bool ElementPictureFactory::build(const ElementsLocation &location, painter.end(); low_painter.end(); - const auto uuid_ = location.uuid(); + const auto uuid_ = cacheKey(location); if (!picture) { m_pictures_H.insert(uuid_, pic); m_primitives_H.insert(uuid_, primitives_); diff --git a/sources/factory/elementpicturefactory.h b/sources/factory/elementpicturefactory.h index 63536a38a..f840a9dc8 100644 --- a/sources/factory/elementpicturefactory.h +++ b/sources/factory/elementpicturefactory.h @@ -92,6 +92,7 @@ class ElementPictureFactory ElementPictureFactory operator= (const ElementPictureFactory &); ~ElementPictureFactory(); + static QUuid cacheKey(const ElementsLocation &location); bool build(const ElementsLocation &location, QPicture *picture=nullptr, QPicture *low_picture=nullptr); void parseElement(const QDomElement &dom, QPainter &painter, primitives &prim) const; void parseLine (const QDomElement &dom, QPainter &painter, primitives &prim) const;