D21788: Make Plasma::Svg::elementRect a bit leaner

[Aleix Pol Gonzalez]

This revision was automatically updated to reflect the committed changes.
Closed by commit R242:097f0e83e7bf: Make Plasma::Svg::elementRect a bit leaner 
(authored by apol).

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21788?vs=59837=60234

REVISION DETAIL
  https://phabricator.kde.org/D21788

AFFECTED FILES
  src/plasma/private/svg_p.h
  src/plasma/svg.cpp

To: apol, #plasma, #frameworks, davidedmundson
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21788: Make Plasma::Svg::elementRect a bit leaner

[David Edmundson]

davidedmundson accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D21788

To: apol, #plasma, #frameworks, davidedmundson
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21788: Make Plasma::Svg::elementRect a bit leaner

[Aleix Pol Gonzalez]

apol added a comment.


  ping?

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D21788

To: apol, #plasma, #frameworks
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21788: Make Plasma::Svg::elementRect a bit leaner

[Aleix Pol Gonzalez]

apol marked an inline comment as done.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D21788

To: apol, #plasma, #frameworks
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21788: Make Plasma::Svg::elementRect a bit leaner

[Aleix Pol Gonzalez]

apol updated this revision to Diff 59837.
apol added a comment.


  static const to be sure that QRegularExpression is only initialised once

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21788?vs=59772=59837

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D21788

AFFECTED FILES
  src/plasma/private/svg_p.h
  src/plasma/svg.cpp

To: apol, #plasma, #frameworks
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21788: Make Plasma::Svg::elementRect a bit leaner

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> apol wrote in svg.cpp:123
> pcre2 does it, AFAIK.

Benchmark tells `const static` is faster:

F6890682: qre_bench.cpp 

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D21788

To: apol, #plasma, #frameworks
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21788: Make Plasma::Svg::elementRect a bit leaner

[Aleix Pol Gonzalez]

apol added inline comments.

INLINE COMMENTS

> bruns wrote in svg.cpp:123
> Are you sure about this? As far as I can see:
> 
> `QRegularExpression::QRegularExpression(QString pattern)` default constructs 
> a `QRegularExpressionPrivate`, which sets `dirty(true)`.
> 
> Calling `QRE::globalMatch()` calls `QREPrivate::compilePattern`, which calls 
> `pcre2_compile_16` and `pcre2_jit_compile_16`.
> 
> https://code.woboq.org/qt5/qtbase/src/corelib/tools/qregularexpression.cpp.html#_ZN25QRegularExpressionPrivate14compilePatternEv
> 
> I don't see any caching here.

pcre2 does it, AFAIK.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D21788

To: apol, #plasma, #frameworks
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21788: Make Plasma::Svg::elementRect a bit leaner

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> apol wrote in svg.cpp:123
> static isn't necessary, QRegularExpression already has internal regex 
> compilation optimisations we can leverage.
> Does the assert bother you?

Are you sure about this? As far as I can see:

`QRegularExpression::QRegularExpression(QString pattern)` default constructs a 
`QRegularExpressionPrivate`, which sets `dirty(true)`.

Calling `QRE::globalMatch()` calls `QREPrivate::compilePattern`, which calls 
`pcre2_compile_16` and `pcre2_jit_compile_16`.

https://code.woboq.org/qt5/qtbase/src/corelib/tools/qregularexpression.cpp.html#_ZN25QRegularExpressionPrivate14compilePatternEv

I don't see any caching here.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D21788

To: apol, #plasma, #frameworks
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21788: Make Plasma::Svg::elementRect a bit leaner

[Aleix Pol Gonzalez]

apol marked an inline comment as done.
apol added inline comments.

INLINE COMMENTS

> bruns wrote in svg.cpp:123
> Are you sure you uploaded the right diff? No const static, Q_ASSERT ...

static isn't necessary, QRegularExpression already has internal regex 
compilation optimisations we can leverage.
Does the assert bother you?

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D21788

To: apol, #plasma, #frameworks
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21788: Make Plasma::Svg::elementRect a bit leaner

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> svg.cpp:123
> +QRegularExpression 
> idExpr(QLatin1String("id\\s*?=\\s*?(['\"])(\\d+?-\\d+?-.*?)\\1"));
> +Q_ASSERT(idExpr.isValid());
>  

Are you sure you uploaded the right diff? No const static, Q_ASSERT ...

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D21788

To: apol, #plasma, #frameworks
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21788: Make Plasma::Svg::elementRect a bit leaner

[Aleix Pol Gonzalez]

apol updated this revision to Diff 59772.
apol added a comment.


  Prefer a QRegularExpression to QRegExp

REPOSITORY
  R242 Plasma Framework (Library)

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21788?vs=59746=59772

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D21788

AFFECTED FILES
  src/plasma/private/svg_p.h
  src/plasma/svg.cpp

To: apol, #plasma, #frameworks
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21788: Make Plasma::Svg::elementRect a bit leaner

[Stefan Brüns]

bruns added a comment.


  Please put the dead code removal in a separate review.

INLINE COMMENTS

> svg.cpp:111
>  const QString contentsAsString(QString::fromLatin1(contents));
> -QRegExp idExpr(QLatin1String("id\\s*=\\s*(['\"])(\\d+-\\d+-.*)\\1"));
> +static QRegExp 
> idExpr(QLatin1String("id\\s*=\\s*(['\"])(\\d+-\\d+-.*)\\1"));
>  idExpr.setMinimal(true);

Please use a QRegularExpression with lazy quantifiers, you can use `const 
static` then, no need for `QRegExp.setMinimal(true)`.
Just replace `*`/`+` with `*?`/`+?`.

REPOSITORY
  R242 Plasma Framework (Library)

REVISION DETAIL
  https://phabricator.kde.org/D21788

To: apol, #plasma, #frameworks
Cc: bruns, kde-frameworks-devel, LeGast00n, michaelh, ngraham

D21788: Make Plasma::Svg::elementRect a bit leaner

[Aleix Pol Gonzalez]

apol created this revision.
apol added reviewers: Plasma, Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
apol requested review of this revision.

REVISION SUMMARY
  Don't double-access maps
  Don't construct ids more times than necessary.
  Make regular expression static so it's not compiled on every run.
  Remove unused code.

REPOSITORY
  R242 Plasma Framework (Library)

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D21788

AFFECTED FILES
  src/plasma/private/svg_p.h
  src/plasma/svg.cpp

To: apol, #plasma, #frameworks
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns